ahuang98 commented on code in PR #18240:
URL: https://github.com/apache/kafka/pull/18240#discussion_r1897013977
##########
raft/src/main/java/org/apache/kafka/raft/UnattachedState.java:
##########
@@ -71,13 +73,7 @@ public UnattachedState(
@Override
public ElectionState election() {
- if (votedKey.isPresent()) {
- return ElectionState.withVotedCandidate(epoch, votedKey().get(),
voters);
- } else if (leaderId.isPresent()) {
- return ElectionState.withElectedLeader(epoch, leaderId.getAsInt(),
voters);
- } else {
- return ElectionState.withUnknownLeader(epoch, voters);
- }
+ return new ElectionState(epoch, leaderId, votedKey, voters);
Review Comment:
I think I had convinced myself this was arguably not an invariant since it
is not enforced (and perhaps just an unintentional quality of KRaft today).
I know we chatted earlier about how KRaftVersion=2 should enforce that both
votedKey and leaderId are persisted if they exist, which I agree with. I guess
what we're not on the same page about is if we can start to persist all the
information we have, now. I would argue it is fine to do now because it is
backwards compatible, the additional info isn't necessary for correctness, and
losing that additional information also doesn't affect correctness (e.g.
currently, if Unattached with leaderId grants a standard vote, it transition to
UnattachedVoted w/o leaderId. UnattachedVoted w/o leaderId and UnattachedVoted
w/ leaderId behave the same way in voting. If Unattached has both leaderId and
votedKey in electionState and then downgrades, it would only retain the
votedKey and transition to UnattachedVoted w/o leader, which is the same
transition that would have been taken in the past for an Unattached w/ leaderId
that grants a vote)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
