You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "CalvinConfluent (via GitHub)" <gi...@apache.org> on 2025/07/19 05:41:51 UTC
[PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
CalvinConfluent opened a new pull request, #20200:
URL: https://github.com/apache/kafka/pull/20200
https://issues.apache.org/jira/browse/KAFKA-19522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217519557
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1233,4 +1233,72 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
// No change to the partition.
assertEquals(Optional.empty(), builder.build());
}
+
+ @Test
Review Comment:
```suggestion
@EnumSource(value = Election.class, names = {"PREFERRED", "UNCLEAN"})
@ParameterizedTest
public void testEligibleLeaderReplicas_NotEligibleLastKnownLeader(Election election) {
```
Please also change `.setElection(Election.PREFERRED)` to `.setElection(election)`
In this way, we don't need another similar test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217212131
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1230,6 +1230,40 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
builder.setTargetIsr(List.of());
+ // No change to the partition.
+ assertEquals(Optional.empty(), builder.build());
+ }
+
+ @Test
+ public void testEligibleLeaderReplicas_NotEligiblableLastKnownLeader() {
Review Comment:
```suggestion
public void testEligibleLeaderReplicas_NotEligibleLastKnownLeader() {
```
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1230,6 +1230,40 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
builder.setTargetIsr(List.of());
+ // No change to the partition.
+ assertEquals(Optional.empty(), builder.build());
+ }
+
+ @Test
+ public void testEligibleLeaderReplicas_NotEligiblableLastKnownLeader() {
+ short version = 2;
+ PartitionRegistration partition = new PartitionRegistration.Builder()
+ .setReplicas(new int[] {1, 2, 3, 4})
+ .setDirectories(new Uuid[]{
+ Uuid.fromString("zANDdMukTEqefOvHpmniMg"),
+ Uuid.fromString("Ui2Eq8rbRiuW7m7uiPTRyg"),
+ Uuid.fromString("MhgJOZrrTsKNcGM0XKK4aA"),
+ Uuid.fromString("Y25PaCAmRfyGIKxAThhBAw")
+ })
+ .setIsr(new int[] {})
+ .setElr(new int[] {})
+ .setLastKnownElr(new int[] {1})
+ .setLeader(-1)
+ .setLeaderRecoveryState(LeaderRecoveryState.RECOVERED)
+ .setLeaderEpoch(100)
+ .setPartitionEpoch(200)
+ .build();
+ Uuid topicId = Uuid.fromString("FbrrdcfiR-KC2CPSTHaJrg");
+
+ PartitionChangeBuilder builder = new PartitionChangeBuilder(partition, topicId, 0, r -> false,
+ metadataVersionForPartitionChangeRecordVersion(version), 3)
+ .setElection(Election.PREFERRED)
Review Comment:
Should we add a scenario for `Election.UNCLEAN`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217850586
##########
metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java:
##########
@@ -311,9 +311,10 @@ private boolean canElectLastKnownLeader() {
topicId, partitionId, Arrays.toString(partition.lastKnownElr));
return false;
}
- if (isAcceptableLeader.test(partition.lastKnownElr[0])) {
+ if (!isAcceptableLeader.test(partition.lastKnownElr[0])) {
log.trace("Try to elect last known leader for {}-{} but last known leader is not alive. last known leader={}",
Review Comment:
Sorry for late reviews.
It seems this comment is not totally accurate. The `isAcceptableLeader` could reject the leader if the leader, while still alive, has no available directory for the partition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217208569
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1230,6 +1230,40 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
builder.setTargetIsr(List.of());
+ // No change to the partition.
+ assertEquals(Optional.empty(), builder.build());
+ }
+
+ @Test
Review Comment:
This leads to style checks failing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #20200:
URL: https://github.com/apache/kafka/pull/20200#issuecomment-3094373341
Cherry-pick to 4.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217241768
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1230,6 +1230,40 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
builder.setTargetIsr(List.of());
+ // No change to the partition.
+ assertEquals(Optional.empty(), builder.build());
+ }
+
+ @Test
+ public void testEligibleLeaderReplicas_NotEligiblableLastKnownLeader() {
+ short version = 2;
+ PartitionRegistration partition = new PartitionRegistration.Builder()
+ .setReplicas(new int[] {1, 2, 3, 4})
+ .setDirectories(new Uuid[]{
+ Uuid.fromString("zANDdMukTEqefOvHpmniMg"),
+ Uuid.fromString("Ui2Eq8rbRiuW7m7uiPTRyg"),
+ Uuid.fromString("MhgJOZrrTsKNcGM0XKK4aA"),
+ Uuid.fromString("Y25PaCAmRfyGIKxAThhBAw")
+ })
+ .setIsr(new int[] {})
+ .setElr(new int[] {})
+ .setLastKnownElr(new int[] {1})
+ .setLeader(-1)
+ .setLeaderRecoveryState(LeaderRecoveryState.RECOVERED)
+ .setLeaderEpoch(100)
+ .setPartitionEpoch(200)
+ .build();
+ Uuid topicId = Uuid.fromString("FbrrdcfiR-KC2CPSTHaJrg");
+
+ PartitionChangeBuilder builder = new PartitionChangeBuilder(partition, topicId, 0, r -> false,
+ metadataVersionForPartitionChangeRecordVersion(version), 3)
+ .setElection(Election.PREFERRED)
Review Comment:
Should we add a test case for `Election.UNCLEAN`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "CalvinConfluent (via GitHub)" <gi...@apache.org>.
CalvinConfluent commented on PR #20200:
URL: https://github.com/apache/kafka/pull/20200#issuecomment-3092636182
@junrao @frankvicky If 4.1 is still open, we should merge this change. We should be able to close this PR pretty soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #20200:
URL: https://github.com/apache/kafka/pull/20200#discussion_r2217519808
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -1233,4 +1233,72 @@ public void testEligibleLeaderReplicas_ElectLastKnownLeaderShouldFail() {
// No change to the partition.
assertEquals(Optional.empty(), builder.build());
}
+
+ @Test
+ public void testEligibleLeaderReplicas_NotEligibleLastKnownLeader() {
+ short version = 2;
+ PartitionRegistration partition = new PartitionRegistration.Builder()
+ .setReplicas(new int[] {1, 2, 3, 4})
+ .setDirectories(new Uuid[]{
+ Uuid.fromString("zANDdMukTEqefOvHpmniMg"),
+ Uuid.fromString("Ui2Eq8rbRiuW7m7uiPTRyg"),
+ Uuid.fromString("MhgJOZrrTsKNcGM0XKK4aA"),
+ Uuid.fromString("Y25PaCAmRfyGIKxAThhBAw")
+ })
+ .setIsr(new int[] {})
+ .setElr(new int[] {})
+ .setLastKnownElr(new int[] {1})
+ .setLeader(-1)
+ .setLeaderRecoveryState(LeaderRecoveryState.RECOVERED)
+ .setLeaderEpoch(100)
+ .setPartitionEpoch(200)
+ .build();
+ Uuid topicId = Uuid.fromString("FbrrdcfiR-KC2CPSTHaJrg");
+
+ PartitionChangeBuilder builder = new PartitionChangeBuilder(partition, topicId, 0, r -> false,
+ metadataVersionForPartitionChangeRecordVersion(version), 3)
+ .setElection(Election.PREFERRED)
+ .setEligibleLeaderReplicasEnabled(true)
+ .setDefaultDirProvider(DEFAULT_DIR_PROVIDER)
+ .setUseLastKnownLeaderInBalancedRecovery(true);
+
+ builder.setTargetIsr(List.of());
+
+ // No change to the partition.
+ assertEquals(Optional.empty(), builder.build());
+ }
+
+ @Test
+ public void testUncleanLeaderElectionFailureIfNoAvailableReplica() {
+ short version = 2;
+ PartitionRegistration partition = new PartitionRegistration.Builder()
+ .setReplicas(new int[] {1, 2, 3, 4})
+ .setDirectories(new Uuid[]{
+ Uuid.fromString("zANDdMukTEqefOvHpmniMg"),
+ Uuid.fromString("Ui2Eq8rbRiuW7m7uiPTRyg"),
+ Uuid.fromString("MhgJOZrrTsKNcGM0XKK4aA"),
+ Uuid.fromString("Y25PaCAmRfyGIKxAThhBAw")
+ })
+ .setIsr(new int[] {})
+ .setElr(new int[] {})
+ .setLastKnownElr(new int[] {1})
+ .setLeader(-1)
+ .setLeaderRecoveryState(LeaderRecoveryState.RECOVERED)
+ .setLeaderEpoch(100)
+ .setPartitionEpoch(200)
+ .build();
+ Uuid topicId = Uuid.fromString("FbrrdcfiR-KC2CPSTHaJrg");
+
+ PartitionChangeBuilder builder = new PartitionChangeBuilder(partition, topicId, 0, r -> false,
+ metadataVersionForPartitionChangeRecordVersion(version), 3)
+ .setElection(Election.UNCLEAN)
Review Comment:
The two new tests are identical except for the election method. Could we make a parameterized test that covers both?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19522: avoid electing fenced lastKnownLeader [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky merged PR #20200:
URL: https://github.com/apache/kafka/pull/20200
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org