ahuang98 commented on code in PR #19590:
URL: https://github.com/apache/kafka/pull/19590#discussion_r2066930477
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -165,8 +165,20 @@ public PartitionRegistration build() {
public final int leaderEpoch;
public final int partitionEpoch;
- public static boolean electionWasClean(int newLeader, int[] isr, int[]
elr) {
- return newLeader == NO_LEADER || Replicas.contains(isr, newLeader) ||
Replicas.contains(elr, newLeader);
+ public static boolean electionWasClean(PartitionRegistration prev,
PartitionRegistration next) {
+ int newLeader = next.leader;
+ // take current all replicas as ISR if prev is null (new created
partition), so we won't treat it as unclean election.
Review Comment:
nit: let's clarify the wording while we're here?
`take all current replicas...`
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -165,8 +165,20 @@ public PartitionRegistration build() {
public final int leaderEpoch;
public final int partitionEpoch;
- public static boolean electionWasClean(int newLeader, int[] isr, int[]
elr) {
- return newLeader == NO_LEADER || Replicas.contains(isr, newLeader) ||
Replicas.contains(elr, newLeader);
+ public static boolean electionWasClean(PartitionRegistration prev,
PartitionRegistration next) {
+ int newLeader = next.leader;
+ // take current all replicas as ISR if prev is null (new created
partition), so we won't treat it as unclean election.
+ int[] prevIsr = prev != null ? prev.isr : next.replicas;
+ int[] prevElr = prev != null ? prev.elr : new int[]{};
+ int[] prevReplicas = prev != null ? prev.replicas : next.replicas;
+ boolean isReassignment = !Arrays.equals(prevReplicas, next.replicas);
+ // A reassignment can change the partition replicas, which also means
it can change the preferred leader.
+ // When all the replicas required in the reassignment task is added to
ISR, the reassignment will be completed.
+ // However, if the new preferred leader is the last one added to ISR,
it will be also elected in the same
+ // partition change. In this case, the new leader will not be in the
previous ISR.
Review Comment:
`In this case, even though the new leader will not be in the previous ISR
this should be treated as a clean election`?
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -165,8 +165,20 @@ public PartitionRegistration build() {
public final int leaderEpoch;
public final int partitionEpoch;
- public static boolean electionWasClean(int newLeader, int[] isr, int[]
elr) {
- return newLeader == NO_LEADER || Replicas.contains(isr, newLeader) ||
Replicas.contains(elr, newLeader);
+ public static boolean electionWasClean(PartitionRegistration prev,
PartitionRegistration next) {
+ int newLeader = next.leader;
+ // take current all replicas as ISR if prev is null (new created
partition), so we won't treat it as unclean election.
+ int[] prevIsr = prev != null ? prev.isr : next.replicas;
+ int[] prevElr = prev != null ? prev.elr : new int[]{};
+ int[] prevReplicas = prev != null ? prev.replicas : next.replicas;
+ boolean isReassignment = !Arrays.equals(prevReplicas, next.replicas);
+ // A reassignment can change the partition replicas, which also means
it can change the preferred leader.
+ // When all the replicas required in the reassignment task is added to
ISR, the reassignment will be completed.
+ // However, if the new preferred leader is the last one added to ISR,
it will be also elected in the same
+ // partition change. In this case, the new leader will not be in the
previous ISR.
+ // During a real unclean leader election, the reassignment will not
complete.
+ return newLeader == NO_LEADER || Replicas.contains(prevIsr, newLeader)
|| Replicas.contains(prevElr, newLeader) ||
+ isReassignment && Replicas.contains(next.isr, newLeader);
Review Comment:
just double checking, if `newLeader` was not in the previous replica list,
is it _always_ true that it will not be in `next.isr`? I'm trying to understand
if it's possible to get a false negative where we report an unclean election as
clean
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
