MINOR: Cleanup Raft Module #20348
MINOR: Cleanup Raft Module #20348
Conversation
|
Adding @chia7712 as he has been kindly helping review a whole lot of PRs in this domain. |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
| public class LeaderAndEpoch { | ||
| private final OptionalInt leaderId; | ||
| private final int epoch; | ||
| public record LeaderAndEpoch(OptionalInt leaderId, int epoch) { | ||
| public static final LeaderAndEpoch UNKNOWN = new LeaderAndEpoch(OptionalInt.empty(), 0); | ||
|
|
||
| public LeaderAndEpoch(OptionalInt leaderId, int epoch) { |
There was a problem hiding this comment.
this could be simplified.
public LeaderAndEpoch {
Objects.requireNonNull(leaderId);
}Also, could you please add unit test for it?
There was a problem hiding this comment.
Make sense. I have added LeaderAndEpochTest for the same.
| @@ -28,7 +28,7 @@ | |||
| * Tracks the votes cast by voters in an election held by a Nominee. | |||
| */ | |||
| public class EpochElection { | |||
There was a problem hiding this comment.
it could be a record class, right?
There was a problem hiding this comment.
I don't think we can do so here. Right now EpochElection holds mutable state voterStates and the methods like recordVote(...) mutate it (voterState.setState(...)). As far as I know, record only supports final members.
There was a problem hiding this comment.
mutable state voterStates
voterStates is changed to a "final" member in this PR, so EpochElection could be a record class now.
public record EpochElection(Map<Integer, VoterState> voterStates) {
public EpochElection(Set<ReplicaKey> voterStates) {
this(voterStates.stream().collect(Collectors.toMap(ReplicaKey::id, VoterState::new)));
}There was a problem hiding this comment.
Yes makes sense. I have fixed the same. Pls re-review when possible.
| public class EpochElection { | ||
| private Map<Integer, VoterState> voterStates; | ||
|
|
||
| public record EpochElection(Map<Integer, VoterState> voterStates) { |
There was a problem hiding this comment.
Could you please make voterStates immutable?
There was a problem hiding this comment.
Also, the toString could be removed
There was a problem hiding this comment.
the voterIds could return voterStates.keySet() instead if voterStates is immutable.
There was a problem hiding this comment.
I removed the toString()
For the other comment on making voterStates immutable, I am not sure if that can be done. The public constructor EpochElection is what is used by multiple classes and it takes Set<ReplicaKey> voters and generates the needed Map with VoterState inner class.
This PR aims at cleaning up the
raftmodule further by getting rid ofsome extra code which can be replaced by
recordReviewers: Chia-Ping Tsai [email protected]