-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an option to disable write stall #251
Conversation
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall.
Have you tested both of disable_write_stall
and base_background_compactions
are dynamic changeable?
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Add two test cases to cover the two configs. |
@@ -829,7 +829,7 @@ WriteStallCondition ColumnFamilyData::RecalculateWriteStallConditions( | |||
name_.c_str(), vstorage->estimated_compaction_needed_bytes(), | |||
write_controller->delayed_write_rate()); | |||
} else { | |||
assert(write_stall_condition == WriteStallCondition::kNormal); | |||
assert(write_stall_condition == WriteStallCondition::kNormal || mutable_cf_options.disable_write_stall); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Connor1996 Minor: should be else if (write_stall_condition == WriteStallCondition::kNormal)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each branch has its own precondition, which is "write_stall_condition
is unset" here. For this specific spot, breaking the precondition might not cause any trouble, but it's a mistake nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable_write_stall
is a kind of "write_stall_condition is unset", using else if
needs to duplicate the logic in else
branch.
* add option to disable write stall Signed-off-by: Connor1996 <zbk602423539@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com>
…#258) * Revert "Add an option to disable write stall (#251) (#252)" This reverts commit 32b29a1. Signed-off-by: tabokie <xy.tao@outlook.com> * use ignore_unknown_options regardless of RocksDB version Signed-off-by: tabokie <xy.tao@outlook.com> * disable only for lower version and fix test Signed-off-by: tabokie <xy.tao@outlook.com>
* add option to disable write stall Signed-off-by: Connor1996 <zbk602423539@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com>
* add option to disable write stall Signed-off-by: Connor1996 <zbk602423539@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com>
* add option to disable write stall Signed-off-by: Connor1996 <zbk602423539@gmail.com> Signed-off-by: tabokie <xy.tao@outlook.com>
disable-write-stall
to disable write stall. It avoids RocksDB from being the write stall state, but still keeps the logic of decreasing/increasing compaction threads unchanged as before.base_background_compactions
to decidemax_compactions
when decrease/increase compaction threads.