config: add "legacy defaults" capability and decrease partitions per shard default #9197
Conversation
This file contains 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
This is a mechanism for us to set improved default values for cluster configuration properties, without impacting already-deployed systems. Systems with an older original_version in the feature table will apply the legacy default.
This enables the configuration object to apply any special legacy-only defaults
...while leaving a legacy default to retain the old value for old clusters, in case anyone was running a special system with more partitions than this (e.g. on very fast CPU cores that can handle it).
This relied on the legacy 7000 partitions per shard default.
jcsp
force-pushed
the
config-legacy-defaults
branch
from
4fdc45f
to
04c076e
Compare
andrwng
reviewed
Mar 2, 2023
| @@ -88,16 +88,18 @@ def test_cpu_limited(self): | |||
|
|
|||
| rpk = RpkTool(self.redpanda) | |||
|
|
|||
| # Three nodes, each with 1 core, 7000 partition-replicas | |||
| # per core, so with replicas=3, 7000 partitions should be the limit | |||
| # Three nodes, each with 1 core, 1000 partition-replicas | |||
There was a problem hiding this comment.
Not necessarily this test, but it'd be great to add an upgrade test that validates that this config doesn't change over jumping from 23.1.1.
|
|
||
| // An alternative default that applies if the cluster's original logical | ||
| // version is <= the defined version | ||
| const std::optional<legacy_default<T>> _legacy_default; |
There was a problem hiding this comment.
Doesn't have to be now, but I can imagine this eventually evolving into a list of defaults as we continue to tune certain defaults.
There was a problem hiding this comment.
Yeah, should be quick to add as/when we need it.
andrwng
approved these changes
Mar 2, 2023
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.
Historically it has been tough to change certain config defaults because it may be unsafe to e.g. decrease a limit if there are systems in the wild that are already beyond the new limit.
This PR adds a new mechanism where early in startup, the feature table notifies the configuration object of the original logical version, and the configuration properties may override their
_defaultbased on that version.That mechanism is then used to reduce the partitions per shard default to 1000. This change is important because the current limits are unsafe, both for raw partition count issues, and also for the default limits on tiered storage reader limits, which are driven by the max partition count.
The main limitation to the mechanism used here is that it will only help with configs that are read after cluster bootstrap happens, or in the case of a node joining a cluster, after the new node sees initial controller log messages. This is suitable for the particular config being handled in this PR, but it is important that any future uses of the mechanism bear this in mind (there is a boldface comment by the definition of
legacy_versionstruct)Fixes #9179
Backports Required
Release Notes
Improvements
topic_partitions_per_shard) by default. Legacy clusters will continue to use the legacy limit (7000), although it is recommended to modify configuration on legacy clusters to use a lower limit, to avoid risk of instability if excessive partitions are created.