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
