config: make all backlog_controller properties needs_restart::no
#27106
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillemKauf
force-pushed
the
pid_no_restart
branch
from
3673d3c to
5119cff
Compare
…ng`s The two use cases of the `backlog_controller` in the code presently are the `compaction_controller` and the `upload_controller`. In both cases, there are cluster properties associated with the PID values used in the `backlog_controller`, which presently require a restart to take effect. There is no strong reason for this to be the case. To enable making these values changeable at run-time, alter the `backlog_controller_config` and use of it in the `backlog_controller` to instead use `config::binding`s around the raw value types. While this does throw away some of the bare value type simplicity of the `backlog_controller` design, the benefits and actual use cases of this class in the codebase are a good motivation for making this change.
The properties for the `upload_controller` and `compaction_controller` can now be marked as not needing a restart to take effect, since all of their uses are now bindings.
WillemKauf
force-pushed
the
pid_no_restart
branch
from
5119cff to
9b1b149
Compare
WillemKauf
commented
Aug 2, 2025
| (space_info.capacity / 100) * backlog_capacity_percents | ||
| / ss::smp::count); | ||
|
|
||
| auto backlog_size_function = [capacity = space_info.capacity] { |
There was a problem hiding this comment.
I wonder if this (like the upload_controller_config) should use available disk space as a setpoint/normalization factor instead of capacity 🤔
WillemKauf
commented
Aug 2, 2025
| = ss::fs_avail(config::node().data_directory().path.string()).get(); | ||
| int64_t setpoint = 0; | ||
| int64_t normalization = static_cast<int64_t>(available) | ||
| auto space_info = std::filesystem::space( |
There was a problem hiding this comment.
needs to be a synchronous call here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The two use cases of the
backlog_controllerin the code presently are thecompaction_controllerand theupload_controller. In both cases, there are cluster properties associated with the PID values used in thebacklog_controller_config, which presently require a restart to take effect.There is no strong reason for this to be the case. To enable making these values changeable at run-time, alter the
backlog_controller_configand use of it in thebacklog_controllerto instead useconfig::bindings around the raw value types.While this does throw away some of the bare value type simplicity of the
backlog_controllerdesign, the benefits and actual use cases of this class in the codebase are a good motivation for making this change.The properties for the
upload_controllerandcompaction_controllercan now be marked as not needing a restart to take effect, since all of their uses are now bindings.Backports Required
Release Notes