Skip to content
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

MINOR: Move the ELR default version to 4.1 #18954

Merged
merged 12 commits into from
Feb 21, 2025

Conversation

CalvinConfluent
Copy link
Contributor

@CalvinConfluent CalvinConfluent commented Feb 18, 2025

ELR is enabled (ELRV_1) by default if the cluster is created with its bootstrap metadata version >= IBP_4_1_IV0.
ELRV_1 can be manually enabled iff the metadata version is >= IBP_4_0_IV1.

@github-actions github-actions bot added triage PRs from the community core Kafka Broker kraft small Small PRs labels Feb 18, 2025
@@ -130,7 +130,7 @@ public enum MetadataVersion {
// Please move this comment when updating the LATEST_PRODUCTION constant.
//

// Not used by anything yet.
// The default version to enable ELR (KIP-966).
IBP_4_1_IV0(26, "4.1", "IV0", false);
Copy link
Member

@ijuma ijuma Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ELR related comments for IBP_4_0_IV1 are a bit unclear - can we clarify them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned 4.0IV1 adds the support for ELR and ELR is only for preview

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment should be something like "enables ELR by default for new clusters"

@dajac dajac changed the title Move the ELR default version to 4.1 MINOR: Move the ELR default version to 4.1 Feb 18, 2025
@dajac dajac added ci-approved and removed triage PRs from the community labels Feb 18, 2025
@@ -25,7 +25,7 @@ public enum EligibleLeaderReplicasVersion implements FeatureVersion {
ELRV_0(0, MetadataVersion.MINIMUM_KRAFT_VERSION, Collections.emptyMap()),

// Version 1 enables the ELR (KIP-966).
ELRV_1(1, MetadataVersion.IBP_4_0_IV1, Collections.emptyMap());
ELRV_1(1, MetadataVersion.IBP_4_1_IV0, Collections.emptyMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this will work since it will disable ELR completely for 4.0-IV1.

In any case, we need a test of ELR working with 4.0-IV1.

@@ -25,7 +25,7 @@ public enum EligibleLeaderReplicasVersion implements FeatureVersion {
ELRV_0(0, MetadataVersion.MINIMUM_KRAFT_VERSION, Collections.emptyMap()),

// Version 1 enables the ELR (KIP-966).
ELRV_1(1, MetadataVersion.IBP_4_0_IV1, Collections.emptyMap());
ELRV_1(1, MetadataVersion.IBP_4_1_IV0, Collections.emptyMap());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to define the dependency on IBP_4_0_IV1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@dajac dajac added the Blocker This pull request is identified as solving a blocker for a release. label Feb 19, 2025
Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will let @cmccabe do the final review and merge it.

@cmccabe
Copy link
Contributor

cmccabe commented Feb 20, 2025

@CalvinConfluent please add a test case to FormatterTest of someone turning on ELR for formatting when MV = 4.0-IV1. Also add a test case that fails with ELR = 1 and MV = 3.9-IV0 (or other earlier MV)

Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.

@dajac
Copy link
Member

dajac commented Feb 21, 2025

QuorumControllerTest > testBalancePartitionLeaders()

This test (Java 23) failed but it passes locally. Looking at the history, it became flaky since a few days ago. Hence it is not related to this patch. Filed https://issues.apache.org/jira/browse/KAFKA-18844.

@dajac dajac merged commit 8f13e7c into apache:trunk Feb 21, 2025
8 of 9 checks passed
dajac pushed a commit that referenced this pull request Feb 21, 2025
- ELR is enabled (ELRV_1) by default if the cluster is created with its bootstrap metadata version >= IBP_4_1_IV0.
- ELRV_1 can be manually enabled iff the metadata version is >= IBP_4_0_IV1.

Reviewers: Ismael Juma <[email protected]>, Colin P. McCabe <[email protected]>, David Jacot <[email protected]>
@dajac
Copy link
Member

dajac commented Feb 21, 2025

Merged to trunk and to 4.0.

@ijuma
Copy link
Member

ijuma commented Feb 21, 2025

Interesting, the flaky test looks related to #18845, I added a comment to the JIRA ticket too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker This pull request is identified as solving a blocker for a release. ci-approved core Kafka Broker kraft small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants