You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/24 09:10:16 UTC
[GitHub] [kafka] dengziming opened a new pull request #10393: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
dengziming opened a new pull request #10393:
URL: https://github.com/apache/kafka/pull/10393
*More detailed description of your change*
1. Add grantVote to `EpochState`
2. Move the if-else in `KafkaRaftCllient.handleVoteRequest` to `EpochState`
3. Add unit-test for `grantVote`
*Summary of testing strategy (including rationale)*
Unit Test
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r607233215
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
As a general rule, methods like this should not log IMO. The calling method should log instead. That is, good to avoid the side effect from the "check" operation. It seems like it was like that before this PR. What was the motivation for changing it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#issuecomment-812770982
@hachikuji , Thank you, hava addressed your comments, and will take a look at KAFKA-12607.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming edited a comment on pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
dengziming edited a comment on pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#issuecomment-812770982
@hachikuji , Thank you, have addressed your comments, and will take a look at KAFKA-12607.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r607430517
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
@ijuma , we logged both vote result and the reject reason in `KafkaRaftClient` before this PR, but different `EpochState` have different reject reason, to reduce the cyclomatic complexity we moved the if-else out of `KafkaRaftClient`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r606332415
##########
File path: raft/src/main/java/org/apache/kafka/raft/UnattachedState.java
##########
@@ -88,6 +93,16 @@ public boolean hasElectionTimeoutExpired(long currentTimeMs) {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+
Review comment:
nit: unneeded newline
##########
File path: raft/src/main/java/org/apache/kafka/raft/FollowerState.java
##########
@@ -139,6 +145,13 @@ public void setFetchingSnapshot(Optional<RawSnapshotWriter> fetchingSnapshot) th
this.fetchingSnapshot = fetchingSnapshot;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ log.debug("Rejecting vote request from candidate {} since we already have a leader {} on that epoch",
Review comment:
nit: ".. in epoch {}"? Similarly for other logs.
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,12 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
Review comment:
Might be worth a comment about the behavior when `candidateId` is equal to `localId`. Although the candidate votes for itself, this vote is implicit and not "granted."
##########
File path: raft/src/main/java/org/apache/kafka/raft/EpochState.java
##########
@@ -25,6 +25,14 @@
return Optional.empty();
}
+ /**
+ * Decide whether to grant a vote to a candidate, it is the responsibility of the caller to invoke
+ * {@link QuorumState##transitionToVoted(int, int)} if vote is granted.
+ *
Review comment:
nit: can you document the parameters?
##########
File path: raft/src/main/java/org/apache/kafka/raft/ResignedState.java
##########
@@ -125,6 +131,12 @@ public long remainingElectionTimeMs(long currentTimeMs) {
return preferredSuccessors;
}
+ @Override
+ public boolean grantVote(int candidateId, Supplier<Boolean> logComparator) {
+ log.debug("Rejecting vote request since we have resigned as candidate/leader in this epoch");
+ return false;
Review comment:
Filed this one: https://issues.apache.org/jira/browse/KAFKA-12607.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r608289621
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
You could return Optional[String] probably where defined string would be the rejection reason. This would mean renaming the methods slightly though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r608289621
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
You could return Optional<String> probably where defined string would be the rejection reason. This would mean renaming the methods slightly though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #10393:
URL: https://github.com/apache/kafka/pull/10393
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r605812919
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -592,44 +592,17 @@ private VoteResponseData handleVoteRequest(
transitionToUnattached(candidateEpoch);
}
- final boolean voteGranted;
- if (quorum.isLeader()) {
- logger.debug("Rejecting vote request {} with epoch {} since we are already leader on that epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isCandidate()) {
- logger.debug("Rejecting vote request {} with epoch {} since we are already candidate on that epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isResigned()) {
- logger.debug("Rejecting vote request {} with epoch {} since we have resigned as candidate/leader in this epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isFollower()) {
- FollowerState state = quorum.followerStateOrThrow();
- logger.debug("Rejecting vote request {} with epoch {} since we already have a leader {} on that epoch",
- request, candidateEpoch, state.leaderId());
- voteGranted = false;
- } else if (quorum.isVoted()) {
- VotedState state = quorum.votedStateOrThrow();
- voteGranted = state.votedId() == candidateId;
-
- if (!voteGranted) {
- logger.debug("Rejecting vote request {} with epoch {} since we already have voted for " +
- "another candidate {} on that epoch", request, candidateEpoch, state.votedId());
- }
- } else if (quorum.isUnattached()) {
+ Supplier<Boolean> logComparator = () -> {
Review comment:
It seems a little simpler to compute this value in all cases and pass it through. We could even consider doing the check up-front and rejecting the vote outright if the epoch/offset is behind us. What do you think?
##########
File path: raft/src/main/java/org/apache/kafka/raft/EpochState.java
##########
@@ -18,13 +18,22 @@
import java.io.Closeable;
import java.util.Optional;
+import java.util.function.Supplier;
public interface EpochState extends Closeable {
default Optional<LogOffsetMetadata> highWatermark() {
return Optional.empty();
}
+ /**
+ * Decide whether to grant a vote to a candidate, it is the responsibility of the caller to invoke
+ * {@link QuorumState##transitionToVoted(int, int)} if vote is granted.
+ *
+ * @return true If grant vote.
+ */
+ boolean grantVote(int candidateId, Supplier<Boolean> logComparator);
Review comment:
Since this does not actually result in any state changes, maybe it would be better to name this `canGrantVote` or perhaps `isVoteAllowed`?
##########
File path: raft/src/main/java/org/apache/kafka/raft/ResignedState.java
##########
@@ -125,6 +131,12 @@ public long remainingElectionTimeMs(long currentTimeMs) {
return preferredSuccessors;
}
+ @Override
+ public boolean grantVote(int candidateId, Supplier<Boolean> logComparator) {
+ log.debug("Rejecting vote request since we have resigned as candidate/leader in this epoch");
+ return false;
Review comment:
I'm sure there must have been a reason for this in the original implementation, but it still surprised me to see it. I think it makes sense for the resigned leader to help another candidate get elected. I'll file a separate JIRA about this.
##########
File path: raft/src/main/java/org/apache/kafka/raft/LeaderState.java
##########
@@ -304,6 +305,12 @@ public String toString() {
}
}
+ @Override
+ public boolean grantVote(int candidateId, Supplier<Boolean> logComparator) {
+ log.debug("Rejecting vote request since we are already leader on that epoch");
Review comment:
Maybe we can improve some of these log messages by adding the `candidateId`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r608209771
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
I think it's a misguided goal to reduce cyclomatic complexity if it results in surprising code. Generally, predicate type methods (`isX` or `canX`) should be callable multiple types without undesired behavior like several log lines. Maybe you can address this in a subsequent PR?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r608286550
##########
File path: raft/src/main/java/org/apache/kafka/raft/CandidateState.java
##########
@@ -235,6 +240,15 @@ public int epoch() {
return highWatermark;
}
+ @Override
+ public boolean canGrantVote(int candidateId, boolean isLogUpToDate) {
+ // Still reject vote request even candidateId = localId, Although the candidate votes for
+ // itself, this vote is implicit and not "granted".
+ log.debug("Rejecting vote request from candidate {} since we are already candidate in epoch {}",
+ candidateId, epoch);
Review comment:
Thank you for reminding me of this principle, maybe a better way to fix this is to return `Tuple<Boolean, String>` to represent "canGrantVote" and "rejectReason", I will try to fix this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10393: KAFKA-12539: Refactor KafkaRaftCllient handleVoteRequest to reduce cyclomatic complexity
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10393:
URL: https://github.com/apache/kafka/pull/10393#discussion_r606258483
##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -592,44 +592,17 @@ private VoteResponseData handleVoteRequest(
transitionToUnattached(candidateEpoch);
}
- final boolean voteGranted;
- if (quorum.isLeader()) {
- logger.debug("Rejecting vote request {} with epoch {} since we are already leader on that epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isCandidate()) {
- logger.debug("Rejecting vote request {} with epoch {} since we are already candidate on that epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isResigned()) {
- logger.debug("Rejecting vote request {} with epoch {} since we have resigned as candidate/leader in this epoch",
- request, candidateEpoch);
- voteGranted = false;
- } else if (quorum.isFollower()) {
- FollowerState state = quorum.followerStateOrThrow();
- logger.debug("Rejecting vote request {} with epoch {} since we already have a leader {} on that epoch",
- request, candidateEpoch, state.leaderId());
- voteGranted = false;
- } else if (quorum.isVoted()) {
- VotedState state = quorum.votedStateOrThrow();
- voteGranted = state.votedId() == candidateId;
-
- if (!voteGranted) {
- logger.debug("Rejecting vote request {} with epoch {} since we already have voted for " +
- "another candidate {} on that epoch", request, candidateEpoch, state.votedId());
- }
- } else if (quorum.isUnattached()) {
+ Supplier<Boolean> logComparator = () -> {
Review comment:
Compute this value in all cases and pass it through seems more clear here.
##########
File path: raft/src/main/java/org/apache/kafka/raft/LeaderState.java
##########
@@ -304,6 +305,12 @@ public String toString() {
}
}
+ @Override
+ public boolean grantVote(int candidateId, Supplier<Boolean> logComparator) {
+ log.debug("Rejecting vote request since we are already leader on that epoch");
Review comment:
Good suggestions!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org