Skip to content

KAFKA-16531: calculate check-quorum when leader is not in voter set#16211

Merged
showuon merged 6 commits intoapache:trunkfrom
showuon:KAFKA-16531
Jun 21, 2024
Merged

KAFKA-16531: calculate check-quorum when leader is not in voter set#16211
showuon merged 6 commits intoapache:trunkfrom
showuon:KAFKA-16531

Conversation

@showuon
Copy link
Copy Markdown
Member

@showuon showuon commented Jun 5, 2024

In the check-quorum calculation, the leader should not assume that it is part of the voter set. This may happen when the leader is removing itself from the voter set. This PR improves it by checking if leader is in the voter set or not, and decide how many follower fetches required. Also add tests.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@showuon showuon added kraft KIP-853 KRaft Controller Membership Changes labels Jun 5, 2024
@showuon showuon marked this pull request as ready for review June 6, 2024 08:36
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@showuon thanks for this patch. two small comments are left. PTAL

// 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: 3 voters, the value will be 2
int majority = (int) ((double) (voterStates.size() + 1) / 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curios. this function will return majority number 2 if there are 4 voters. Is that expected? I assume the corrected number is 3

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! My test didn't work as expected. Also updated the test to catch the issue. Thanks.

Comment thread raft/src/main/java/org/apache/kafka/raft/LeaderState.java Outdated
@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jun 13, 2024

@chia7712 , thanks for the review. PR updated in this commit: 7db45c2 . Thanks.

@jsancio jsancio self-assigned this Jun 13, 2024
@jsancio
Copy link
Copy Markdown
Member

jsancio commented Jun 13, 2024

@showuon can you update the description?

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jun 14, 2024

@showuon can you update the description?

PR description updated. Thanks.

@chia7712
Copy link
Copy Markdown
Member

@showuon could you please fix the build error?

Copy link
Copy Markdown
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @showuon . Reviewed raft/src/main.

Comment thread raft/src/main/java/org/apache/kafka/raft/LeaderState.java
Comment thread raft/src/main/java/org/apache/kafka/raft/LeaderState.java Outdated
Comment thread raft/src/main/java/org/apache/kafka/raft/LeaderState.java
Copy link
Copy Markdown
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @showuon . Thanks for the changes! I'll merge if the tests look good.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jun 21, 2024

Failed tests are unrelated

@showuon showuon merged commit d646a09 into apache:trunk Jun 21, 2024
TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jul 4, 2024
…pache#16211)

In the check-quorum calculation, the leader should not assume that it is part of the voter set. This may happen when the leader is removing itself from the voter set. This PR improves it by checking if leader is in the voter set or not, and decide how many follower fetches required. Also add tests.

Co-authored-by: Colin P. McCabe <cmccabe@apache.org>

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, José Armando García Sancio <jsancio@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

KIP-853 KRaft Controller Membership Changes kraft

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants