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.

Sign up for GitHub

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

Jump to bottom

KAFKA-15109 Ensure the leader epoch bump occurs for older MetadataVersions #13910

Merged
merged 3 commits into from Jun 27, 2023
Merged

KAFKA-15109 Ensure the leader epoch bump occurs for older MetadataVersions #13910

merged 3 commits into from Jun 27, 2023

Conversation

mumrah
Copy link
Contributor

@mumrah mumrah commented Jun 23, 2023 •
edited

This patch fixes a bug introduced in the earlier KAFKA-15109 commit (#13890) where the leader epoch bump would be skipped for older MetadataVersion-s.

@mumrah mumrah requested review from jsancio and cmccabe June 23, 2023 14:08
jsancio
jsancio reviewed Jun 23, 2023
View reviewed changes
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java Outdated
@@ -207,7 +211,7 @@ public void testTriggerLeaderEpochBumpIfNeeded() {
createFooBuilder()
.setTargetIsrWithBrokerStates(
AlterPartitionRequest.newIsrToSimpleNewIsrWithBrokerEpochs(Arrays.asList(2, 1, 3, 4)))
.setBumpLeaderEpochOnIsrShrink(true),
.enableBumpLeaderEpochOnIsrShrink(true),
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Shouldn't this test fail? Let's add a comment explaining what this test is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is an ISR expansion, so we don't expect an epoch bump (I added a comment)

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Outside the scope of this PR but it looks like we are missing some validation. This is adding 4 to the ISR but 4 is not a replica for this partition. cc @ahuang98

@cmccabe
Copy link
Contributor

cmccabe commented Jun 26, 2023 •
edited

It's confusing to have a function named enableBumpLeaderEpochOnIsrShrink that sometimes doesn't actually enable (or disable) the thing, based on some other variables. If we're going to go this route, then I'd say just add back zkMigrationInProgress as a flag.

The other approach would be to have a function like forceBumpLeaderEpochOnIsrShrink() with no arguments that just does what it says. After all, bumping the epoch is always safe so we'd expect code like:

if (zkMigration) {
  builder.forceBumpLeaderEpochOnIsrShrink();
}

By NOT providing a function to disable leader epoch bumping, we can avoid the bug we encountered here.

I think either of these approaches could work. Let me know what you think.

@mumrah mumrah changed the title KAFKA-15109 Don't skip leader epoch bump for older MetadataVersions KAFKA-15109 Ensure the leader epoch bump occurs for older MetadataVersions Jun 26, 2023
cmccabe
cmccabe approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM

jsancio
jsancio reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

One minor suggestions. Looks good otherwise.

metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java Outdated
record.setLeader(partition.leader);
} else if (bumpLeaderEpochOnIsrShrink && !Replicas.contains(targetIsr, partition.isr)) {
} else if (!Replicas.contains(targetIsr, partition.isr) && bumpLeaderEpochOnIsrShrink) {
Copy link
Member

Choose a reason for hiding this comment

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

We should check bumpLeaderEpochOnIsrShrink first as it is faster to compute. It avoids a search through the ISR arrays when it is false (most cases after MV 3.6).

metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java Outdated
@@ -207,7 +211,7 @@ public void testTriggerLeaderEpochBumpIfNeeded() {
createFooBuilder()
.setTargetIsrWithBrokerStates(
AlterPartitionRequest.newIsrToSimpleNewIsrWithBrokerEpochs(Arrays.asList(2, 1, 3, 4)))
.setBumpLeaderEpochOnIsrShrink(true),
.enableBumpLeaderEpochOnIsrShrink(true),
Copy link
Member

Choose a reason for hiding this comment

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

Got it. Outside the scope of this PR but it looks like we are missing some validation. This is adding 4 to the ISR but 4 is not a replica for this partition. cc @ahuang98

@jsancio jsancio added the kraft label Jun 27, 2023
@jsancio jsancio self-assigned this Jun 27, 2023
jsancio
jsancio approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM

@mumrah mumrah merged commit fc7d912 into apache:trunk Jun 27, 2023
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsancio jsancio jsancio approved these changes

@cmccabe cmccabe cmccabe approved these changes

Assignees

@jsancio jsancio

Labels
kraft
Projects
None yet
Milestone
No milestone
3 participants
@mumrah @cmccabe @jsancio