jsancio commented on code in PR #16211: URL: https://github.com/apache/kafka/pull/16211#discussion_r1639980790
########## raft/src/main/java/org/apache/kafka/raft/LeaderState.java: ########## @@ -132,8 +133,13 @@ public long timeUntilCheckQuorumExpires(long currentTimeMs) { */ public void updateCheckQuorumForFollowingVoter(int id, long currentTimeMs) { updateFetchedVoters(id); - // The majority number of the voters excluding the leader. Ex: 3 voters, the value will be 1 - int majority = voterStates.size() / 2; + // The majority number of the voters. Ex: 2 for 3 voters, 3 for 4 voters... etc. + int majority = (int) Math.ceil((double) (voterStates.size() + 1) / 2); Review Comment: I don't trust conversions to `double` since they are not exact integers. I think you want: ```java int majority = (voterState.size() / 2) + 1; ``` ########## raft/src/main/java/org/apache/kafka/raft/LeaderState.java: ########## @@ -117,9 +117,10 @@ public long timeUntilCheckQuorumExpires(long currentTimeMs) { long remainingMs = checkQuorumTimer.remainingMs(); if (remainingMs == 0) { log.info( - "Did not receive fetch request from the majority of the voters within {}ms. Current fetched voters are {}.", + "Did not receive fetch request from the majority of the voters within {}ms. Current fetched voters are {}, and voters are {}", checkQuorumTimeoutMs, - fetchedVoters); + fetchedVoters, + voterStates.keySet()); Review Comment: Let's print the replica keys (id and directory id) not just the ids. E.g. `votersStates.values().stream.map(ReplicaState::replicaKey)`. Having said, my comment doesn't apply here since trunk doesn't have those changese. I am thinking that maybe we should wait for us to merge https://github.com/apache/kafka/pull/16235. ########## raft/src/main/java/org/apache/kafka/raft/LeaderState.java: ########## @@ -132,8 +133,13 @@ public long timeUntilCheckQuorumExpires(long currentTimeMs) { */ public void updateCheckQuorumForFollowingVoter(int id, long currentTimeMs) { updateFetchedVoters(id); - // The majority number of the voters excluding the leader. Ex: 3 voters, the value will be 1 - int majority = voterStates.size() / 2; + // The majority number of the voters. Ex: 2 for 3 voters, 3 for 4 voters... etc. + int majority = (int) Math.ceil((double) (voterStates.size() + 1) / 2); + // Check if the leader is removed from the voter set + if (voterStates.containsKey(localId)) { + majority = majority - 1; Review Comment: Let's add a comment explaining why we are doing this. This is my understanding: > If the leader is in the voter set since it should be implicitly counted as part of the majority but the leader will never be a member of the `fetchedVoters`. If the leader is not in the voter set, it is not in majority so the majority can only be composed of fetched voters. Do you agree? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org