KAFKA-16540: enforce min.insync.replicas config invariants for ELR #17952
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
|
Thanks for the PR. Thoughts: I think feature control should not have a reference to cluster control. in general many things reference feature control to know the MV and I don’t want them to have to pull in cluster control for that. It would probably be better to have ConfigurationControl handle the updateFeatures call, and have it call into FeatureControl. |
|
Also, I think we should generate the configuration record for the cluster config as part of the activation event rather than right afterwards. That will ensure that it happens before anything else. |
|
Update Summary:
|
|
Can you fix the checkstyle errors? |
cmccabe
reviewed
Dec 16, 2024
metadata/src/main/java/org/apache/kafka/controller/ActivationRecordsGenerator.java
Outdated
Show resolved
Hide resolved
cmccabe
reviewed
Dec 16, 2024
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java
Outdated
Show resolved
Hide resolved
cmccabe
reviewed
Dec 16, 2024
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java
Outdated
Show resolved
Hide resolved
cmccabe
reviewed
Dec 16, 2024
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java
Outdated
Show resolved
Hide resolved
cmccabe
reviewed
Dec 16, 2024
| // Also, it will remove all the broker level min ISR config records. | ||
| void maybeResetMinIsrConfig(List<ApiMessageAndVersion> outputRecords) { | ||
| if (!clusterConfig().containsKey(MIN_IN_SYNC_REPLICAS_CONFIG)) { | ||
| String minIsrDefaultConfigValue = configSchema.getStaticOrDefaultConfig( |
There was a problem hiding this comment.
We should log a message if we are doing this. Also, it seems easier just to create the configuration record directly than call a function here.
There was a problem hiding this comment.
Sounds good, updated.
cmccabe
reviewed
Dec 16, 2024
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java
Outdated
Show resolved
Hide resolved
cmccabe
changed the title
CalvinConfluent
commented
Jan 8, 2025
| @@ -303,6 +339,14 @@ private ApiError validateAlterConfig(ConfigResource configResource, | |||
| if (alterConfigPolicy.isPresent()) { | |||
| alterConfigPolicy.get().validate(new RequestMetadata(configResource, alteredConfigsForAlterConfigPolicyCheck)); | |||
| } | |||
| if (featureControl.isElrFeatureEnabled()) { | |||
There was a problem hiding this comment.
We should remove this part as we have done the filtering above.
| return isElrFeatureEnabled(latestFinalizedFeatures().versionOrDefault(EligibleLeaderReplicasVersion.FEATURE_NAME, (short) 0)); | ||
| } | ||
|
|
||
| public static boolean isElrFeatureEnabled(short elrFeatureLevel) { |
There was a problem hiding this comment.
We can remove this static method, no one uses it any more.
pranavt84
pushed a commit
to pranavt84/kafka
that referenced
this pull request
Jan 27, 2025
…pache#17952) If ELR is enabled, we need to set a cluster-level min.insync.replicas, and remove all broker-level overrides. The reason for this is that if brokers disagree about which partitions are under min ISR, it breaks the KIP-966 replication invariants. In order to enforce this, when the eligible.leader.replicas.version feature is turned on, we automatically remove all broker-level min.insync.replicas overrides, and create the required cluster-level override if needed. Similarly, if the cluster was created with eligible.leader.replicas.version enabled, we create a similar cluster-level record. In both cases, we don't allow setting overrides for individual brokers afterwards, or removing the cluster-level override. Split ActivationRecordsGeneratorTest up into multiple test cases rather than having it be one giant test case. Fix a bug in QuorumControllerTestEnv where we would replay records manually on objects, racing with the active controller thread. Instead, we should simply ensure that the initial bootstrap records contains what we want. Reviewers: Colin P. McCabe <[email protected]>
CalvinConfluent
added a commit
to CalvinConfluent/kafka
that referenced
this pull request
Jan 29, 2025
…pache#17952) If ELR is enabled, we need to set a cluster-level min.insync.replicas, and remove all broker-level overrides. The reason for this is that if brokers disagree about which partitions are under min ISR, it breaks the KIP-966 replication invariants. In order to enforce this, when the eligible.leader.replicas.version feature is turned on, we automatically remove all broker-level min.insync.replicas overrides, and create the required cluster-level override if needed. Similarly, if the cluster was created with eligible.leader.replicas.version enabled, we create a similar cluster-level record. In both cases, we don't allow setting overrides for individual brokers afterwards, or removing the cluster-level override. Split ActivationRecordsGeneratorTest up into multiple test cases rather than having it be one giant test case. Fix a bug in QuorumControllerTestEnv where we would replay records manually on objects, racing with the active controller thread. Instead, we should simply ensure that the initial bootstrap records contains what we want. Reviewers: Colin P. McCabe <[email protected]>
cmccabe
pushed a commit
that referenced
this pull request
Feb 4, 2025
…17952) If ELR is enabled, we need to set a cluster-level min.insync.replicas, and remove all broker-level overrides. The reason for this is that if brokers disagree about which partitions are under min ISR, it breaks the KIP-966 replication invariants. In order to enforce this, when the eligible.leader.replicas.version feature is turned on, we automatically remove all broker-level min.insync.replicas overrides, and create the required cluster-level override if needed. Similarly, if the cluster was created with eligible.leader.replicas.version enabled, we create a similar cluster-level record. In both cases, we don't allow setting overrides for individual brokers afterwards, or removing the cluster-level override. Split ActivationRecordsGeneratorTest up into multiple test cases rather than having it be one giant test case. Fix a bug in QuorumControllerTestEnv where we would replay records manually on objects, racing with the active controller thread. Instead, we should simply ensure that the initial bootstrap records contains what we want. Reviewers: Colin P. McCabe <[email protected]>
manoj-mathivanan
pushed a commit
to manoj-mathivanan/kafka
that referenced
this pull request
Feb 19, 2025
…pache#17952) If ELR is enabled, we need to set a cluster-level min.insync.replicas, and remove all broker-level overrides. The reason for this is that if brokers disagree about which partitions are under min ISR, it breaks the KIP-966 replication invariants. In order to enforce this, when the eligible.leader.replicas.version feature is turned on, we automatically remove all broker-level min.insync.replicas overrides, and create the required cluster-level override if needed. Similarly, if the cluster was created with eligible.leader.replicas.version enabled, we create a similar cluster-level record. In both cases, we don't allow setting overrides for individual brokers afterwards, or removing the cluster-level override. Split ActivationRecordsGeneratorTest up into multiple test cases rather than having it be one giant test case. Fix a bug in QuorumControllerTestEnv where we would replay records manually on objects, racing with the active controller thread. Instead, we should simply ensure that the initial bootstrap records contains what we want. Reviewers: Colin P. McCabe <[email protected]>
chia7712
pushed a commit
that referenced
this pull request
Aug 4, 2025
Along with the change: #17952 ([KIP-966](https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas)), the semantics of `min.insync.replicas` config has small change, and add some constraints. We should document them clearly. Reviewers: Jun Rao <[email protected]>, Calvin Liu <[email protected]>, Mickael Maison <[email protected]>, Paolo Patierno <[email protected]>, Federico Valeri <[email protected]>, Chia-Ping Tsai <[email protected]>
chia7712
pushed a commit
that referenced
this pull request
Aug 4, 2025
Along with the change: #17952 ([KIP-966](https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas)), the semantics of `min.insync.replicas` config has small change, and add some constraints. We should document them clearly. Reviewers: Jun Rao <[email protected]>, Calvin Liu <[email protected]>, Mickael Maison <[email protected]>, Paolo Patierno <[email protected]>, Federico Valeri <[email protected]>, Chia-Ping Tsai <[email protected]>
airlock-confluentinc bot
pushed a commit
to confluentinc/kafka
that referenced
this pull request
Aug 6, 2025
Along with the change: apache#17952 ([KIP-966](https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas)), the semantics of `min.insync.replicas` config has small change, and add some constraints. We should document them clearly. Reviewers: Jun Rao <[email protected]>, Calvin Liu <[email protected]>, Mickael Maison <[email protected]>, Paolo Patierno <[email protected]>, Federico Valeri <[email protected]>, Chia-Ping Tsai <[email protected]>
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.
If ELR is enabled, we need to set a cluster-level min.insync.replicas, and remove all broker-level overrides. The reason for this is that if brokers disagree about which partitions are under min ISR, it breaks the KIP-966 replication invariants. In order to enforce this, when the eligible.leader.replicas.version feature is turned on, we automatically remove all broker-level min.insync.replicas overrides, and create the required cluster-level override if needed. Similarly, if the cluster was created with eligible.leader.replicas.version enabled, we create a similar cluster-level record. In both cases, we don't allow setting overrides for individual brokers afterwards, or removing the cluster-level override.
Split ActivationRecordsGeneratorTest up into multiple test cases rather than having it be one giant test case.
Fix a bug in QuorumControllerTestEnv where we would replay records manually on objects, racing with the active controller thread. Instead, we should simply ensure that the initial bootstrap records contains what we want.