You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "sjhajharia (via GitHub)" <gi...@apache.org> on 2025/08/13 05:20:07 UTC
[PR] MINOR: Cleanup Raft Module [kafka]
sjhajharia opened a new pull request, #20348:
URL: https://github.com/apache/kafka/pull/20348
This PR aims at cleaning up the `raft` module further by getting rid of some extra code which can be replaced by `record`
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2291760939
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -28,7 +28,7 @@
* Tracks the votes cast by voters in an election held by a Nominee.
*/
public class EpochElection {
Review 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.
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20348:
URL: https://github.com/apache/kafka/pull/20348
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2291631707
##########
raft/src/main/java/org/apache/kafka/raft/LeaderAndEpoch.java:
##########
@@ -19,47 +19,15 @@
import java.util.Objects;
import java.util.OptionalInt;
-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) {
Review Comment:
this could be simplified.
```java
public LeaderAndEpoch {
Objects.requireNonNull(leaderId);
}
```
Also, could you please add unit test for it?
##########
raft/src/main/java/org/apache/kafka/raft/DynamicVoter.java:
##########
@@ -118,67 +110,24 @@ public static DynamicVoter parse(String input) {
/**
* Create a new KIP-853 voter.
*
- * @param directoryId The directory ID.
- * @param nodeId The voter ID.
- * @param host The voter hostname or IP address.
- * @param port The voter port.
+ * @param directoryId The directory ID.
+ * @param nodeId The voter ID.
+ * @param host The voter hostname or IP address.
+ * @param port The voter port.
*/
- public DynamicVoter(
- Uuid directoryId,
- int nodeId,
- String host,
- int port
- ) {
- this.directoryId = directoryId;
- this.nodeId = nodeId;
- this.host = host;
- this.port = port;
- }
-
- public Uuid directoryId() {
- return directoryId;
- }
-
- public int nodeId() {
- return nodeId;
- }
-
- public String host() {
- return host;
- }
-
- public int port() {
- return port;
+ public DynamicVoter {
Review Comment:
this default constructor is unnecessary, right?
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -28,7 +28,7 @@
* Tracks the votes cast by voters in an election held by a Nominee.
*/
public class EpochElection {
Review Comment:
it could be a record class, right?
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2291896856
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -28,7 +28,7 @@
* Tracks the votes cast by voters in an election held by a Nominee.
*/
public class EpochElection {
Review Comment:
> mutable state voterStates
`voterStates` is changed to a "final" member in this PR, so `EpochElection` could be a record class now.
```java
public record EpochElection(Map<Integer, VoterState> voterStates) {
public EpochElection(Set<ReplicaKey> voterStates) {
this(voterStates.stream().collect(Collectors.toMap(ReplicaKey::id, VoterState::new)));
}
```
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2292666668
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -28,7 +28,7 @@
* Tracks the votes cast by voters in an election held by a Nominee.
*/
public class EpochElection {
Review Comment:
Yes makes sense. I have fixed the same. Pls re-review when possible.
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on PR #20348:
URL: https://github.com/apache/kafka/pull/20348#issuecomment-3205569531
Adding @chia7712 as he has been kindly helping review a whole lot of PRs in this domain.
TIA!
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2326774624
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -27,17 +27,11 @@
/**
* Tracks the votes cast by voters in an election held by a Nominee.
*/
-public class EpochElection {
- private Map<Integer, VoterState> voterStates;
-
+public record EpochElection(Map<Integer, VoterState> voterStates) {
Review 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 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] MINOR: Cleanup Raft Module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2291758320
##########
raft/src/main/java/org/apache/kafka/raft/LeaderAndEpoch.java:
##########
@@ -19,47 +19,15 @@
import java.util.Objects;
import java.util.OptionalInt;
-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) {
Review Comment:
Make sense. I have added `LeaderAndEpochTest` for the same.
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20348:
URL: https://github.com/apache/kafka/pull/20348#issuecomment-3208864451
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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] MINOR: Cleanup Raft Module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20348:
URL: https://github.com/apache/kafka/pull/20348#discussion_r2296244873
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -27,17 +27,11 @@
/**
* Tracks the votes cast by voters in an election held by a Nominee.
*/
-public class EpochElection {
- private Map<Integer, VoterState> voterStates;
-
+public record EpochElection(Map<Integer, VoterState> voterStates) {
Review Comment:
the `voterIds` could return `voterStates.keySet()` instead if `voterStates` is immutable.
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -27,17 +27,11 @@
/**
* Tracks the votes cast by voters in an election held by a Nominee.
*/
-public class EpochElection {
- private Map<Integer, VoterState> voterStates;
-
+public record EpochElection(Map<Integer, VoterState> voterStates) {
Review Comment:
Could you please make `voterStates` immutable?
##########
raft/src/main/java/org/apache/kafka/raft/internals/EpochElection.java:
##########
@@ -27,17 +27,11 @@
/**
* Tracks the votes cast by voters in an election held by a Nominee.
*/
-public class EpochElection {
- private Map<Integer, VoterState> voterStates;
-
+public record EpochElection(Map<Integer, VoterState> voterStates) {
Review Comment:
Also, the `toString` could be removed
--
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