-
Notifications
You must be signed in to change notification settings - Fork 593
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
write caching - configurations #16924
Conversation
/dt |
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e1636-93c3-4647-b703-644154fe715e:
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e1636-93bc-4acf-aad7-3901eb2cf74c:
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e1636-93c0-4ff3-b62e-5b541a1bc235:
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e169f-f929-488e-bef0-4ef43a7fb63b:
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e169f-f922-44f5-8b1f-7d46ffc91219:
new failures in https://buildkite.com/redpanda/redpanda/builds/45762#018e169f-f92d-4ac0-b53e-211cdff5232d:
new failures in https://buildkite.com/redpanda/redpanda/builds/45851#018e1ce1-2ccd-4ac4-9860-9905b3d87a97:
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45762#018e1636-93bc-4acf-aad7-3901eb2cf74c ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45785#018e1818-d8a8-417c-8e77-45a56dcf6a10 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45809#018e1a5b-c224-4604-9534-741815d5ebff |
model::write_caching_mode::off); | ||
} | ||
auto cluster_default = config::shard_local_cfg().write_caching(); | ||
if (cluster_default == model::write_caching_mode::disabled) { |
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.
Wondering if we should error here or just accept it and log a warn instead.
I'm thinking about the following case: someone enables write caching on some topics, disabled the feature globally, then wants to change the settings on the topics such that when the feature is enabled back it is only applied to one topic rather than all as it was pre-disabling it.
With the current implementation the above workflow won't be possible.
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.
Doing so I think gives an impression to the caller that write caching is enabled.
Eg:
Admin disables write caching globally
User X enables write caching on their topic, command returns success
X thinks write caching is in place when in reality it isn't. Returning an explicit error forces them check that it is disabled globally and cannot be used, wdyt.
} | ||
try { | ||
auto val = boost::lexical_cast<size_t>(it->value.value()); | ||
return val > 0; |
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.
do we always duplicate the validation logic in 2 places like this? also seen this in config_utils.h
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.
Thats true, thats how it is done today. The two cases are topic creation and updating properties, the validators use different signatures but we could probably consolidate the logic somehow by factoring out the common validation logic.
force_push fixed linter errors, tightened the e2e test and squashed a couple of commits. |
f2cd870
to
daf12a1
Compare
force pushed to fix rebase conflicts with dev (which was the last dt failure). |
@@ -49,6 +49,8 @@ class group_manager { | |||
config::binding<std::chrono::milliseconds> election_timeout_ms; | |||
config::binding<std::optional<size_t>> replica_max_not_flushed_bytes; | |||
config::binding<std::chrono::milliseconds> flush_timer_interval_ms; | |||
config::binding<model::write_caching_mode> write_caching; | |||
config::binding<std::chrono::milliseconds> write_caching_flush_ms; |
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.
I'm a bit confused about relation between old and new config properties. AFAIU we are going to deprecate flush_timer_interval_ms
but reuse replica_max_not_flushed_bytes
. Will the old code using replica_max_not_flushed_bytes
be removed after write caching is introduced? Should we then add another binding write_caching_flush_bytes
for consistency and remove the old binding when it is no longer needed?
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.
AFAIU we are going to deprecate flush_timer_interval_ms but reuse replica_max_not_flushed_bytes
correct.
Will the old code using replica_max_not_flushed_bytes be removed after write caching is introduced? Should we then add another binding write_caching_flush_bytes for consistency and remove the old binding when it is no longer needed?
Correct, that is coming in the next PR, tried to keep this PR totally a mechanical change limiting to configs/properties.
Adds cluster configuration write_caching. Accepted values are [on, off, disabled] on/off applies to all topics in the cluster by default unless there is a topic level override via property write.caching (to be added in subsequent commits). write_caching=disabled takes precedence over everything including topic overrides and is intended to be a kill switch if something goes wrong.
write.caching can override write_caching cluster configuration. Acceptable values [on, off] write_caching=disabled takes precedence over write.caching overrides.
raft_replica_max_flush_delay_ms acts as the cluster default for flush.ms. raft_flush_timer_interval_ms will be deprecated in favor of this new configuration in a future PR.
flush.ms topic property overrides raft_replica_max_flush_delay_ms flush.bytes topic property overrides raft_replica_max_pending_flush_bytes flush.ms acceptable values >= 1ms flush.bytes acceptable values > 0
unclear if this is needed, just adding for completeness.
The configuration opptions are very rarely changed, so it is wasteful to compute them for every write request (reconciling cluster + topic properties). This changes keeps a copy of the configs in the consensus class which are updated using notification hooks.
Configuration values as seen by each replica are now a part of the debug dump for the partition.
These errors are not propagated the clients (which needs to be fixed), until then logging in debug mode.
This PR adds all the configurations/tunables needed for the write caching project as described in https://docs.google.com/document/d/1gD2vXeQuv9gslagl6rDxx3XQsBI9BkXT2Ktpz9my2V0/edit#heading=h.5zvj0etnl8iy
The code using them will come as a separate PR, so until that lands these are dummy unused configurations, just splitting them out into a separate PR for reviewers' convenience.
Summary of configurations
Cluster level:
Topic level:
write_caching=disabled is intended to be the feature kill switch that takes precedence over topic overrides.
Release Notes
Features
Backports Required
Release Notes