You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "showuon (via GitHub)" <gi...@apache.org> on 2024/06/05 12:00:50 UTC
[PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
showuon opened a new pull request, #16211:
URL: https://github.com/apache/kafka/pull/16211
This PR is based on https://github.com/apache/kafka/pull/16079 , so putting it as draft.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16211:
URL: https://github.com/apache/kafka/pull/16211#discussion_r1637455387
##########
raft/src/main/java/org/apache/kafka/raft/LeaderState.java:
##########
@@ -111,9 +111,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);
Review Comment:
Updated. Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16211:
URL: https://github.com/apache/kafka/pull/16211#discussion_r1642365909
##########
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:
OK, will review https://github.com/apache/kafka/pull/16235 later.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #16211:
URL: https://github.com/apache/kafka/pull/16211#issuecomment-2167155537
> @showuon can you update the description?
PR description updated. Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #16211:
URL: https://github.com/apache/kafka/pull/16211#issuecomment-2164292176
@chia7712 , thanks for the review. PR updated in this commit: https://github.com/apache/kafka/pull/16211/commits/7db45c2e8f08b4a381717b4c6b7f466bd116cd4e . Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on PR #16211:
URL: https://github.com/apache/kafka/pull/16211#issuecomment-2166246088
@showuon can you update the description?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16211:
URL: https://github.com/apache/kafka/pull/16211#discussion_r1642365430
##########
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:
Looks good to me. Thanks.
##########
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:
That works, too. Updated.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16211:
URL: https://github.com/apache/kafka/pull/16211#discussion_r1630128548
##########
raft/src/main/java/org/apache/kafka/raft/LeaderState.java:
##########
@@ -126,8 +127,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: 3 voters, the value will be 2
+ int majority = (int) ((double) (voterStates.size() + 1) / 2);
Review Comment:
just curios. this function will return majority number `2` if there are 4 voters. Is that expected? I assume the corrected number is `3`
##########
raft/src/main/java/org/apache/kafka/raft/LeaderState.java:
##########
@@ -111,9 +111,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);
Review Comment:
Should it be `voterStates.keySet()`?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16211:
URL: https://github.com/apache/kafka/pull/16211#discussion_r1637455856
##########
raft/src/main/java/org/apache/kafka/raft/LeaderState.java:
##########
@@ -126,8 +127,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: 3 voters, the value will be 2
+ int majority = (int) ((double) (voterStates.size() + 1) / 2);
Review Comment:
Ah, good catch! My test didn't work as expected. Also updated the test to catch the issue. Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16531: calculate check-quorum when leader is not in voter set [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16211:
URL: https://github.com/apache/kafka/pull/16211#issuecomment-2167362750
@showuon could you please fix the build error?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org