dajac commented on code in PR #12181:
URL: https://github.com/apache/kafka/pull/12181#discussion_r880220434
##########
core/src/main/scala/kafka/cluster/Partition.scala:
##########
@@ -846,21 +846,33 @@ class Partition(val topicPartition: TopicPartition,
}
private def needsExpandIsr(followerReplica: Replica): Boolean = {
- canAddReplicaToIsr(followerReplica.brokerId) &&
isFollowerAtHighwatermark(followerReplica)
+ canAddReplicaToIsr(followerReplica.brokerId) &&
isFollowerInSync(followerReplica)
}
private def canAddReplicaToIsr(followerReplicaId: Int): Boolean = {
val current = partitionState
- !current.isInflight && !current.isr.contains(followerReplicaId)
+ !current.isInflight &&
+ !current.isr.contains(followerReplicaId) &&
+ isBrokerIsrEligible(followerReplicaId)
}
- private def isFollowerAtHighwatermark(followerReplica: Replica): Boolean = {
+ private def isFollowerInSync(followerReplica: Replica): Boolean = {
leaderLogIfLocal.exists { leaderLog =>
val followerEndOffset = followerReplica.stateSnapshot.logEndOffset
followerEndOffset >= leaderLog.highWatermark &&
leaderEpochStartOffsetOpt.exists(followerEndOffset >= _)
}
}
+ private def isBrokerIsrEligible(brokerId: Int): Boolean = {
+ // With KRaft, a broker is considered alive if it is unfenced. If it
+ // is fenced or not present, the leader prevent it to be added back
+ // to the ISR.
+ // With ZK, the leader only prevent dead brokers to be added back
+ // to the ISR. This is really unlikely given that replicas are
Review Comment:
I was trying to convince myself that adding this check, when ZK is used, is
OK. My understanding is that `hasAliveBroker` should always be `true` here
because alive brokers in the metadata cache are always updated before the
partition states are. This check is basically not doing much when ZK is used.
--
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]
