[GitHub] [kafka] jsancio commented on a change in pull request #9539: KAFKA-10634: Adding LeaderId to Voters list in LeaderChangeMessage

[GitBox <gi...@apache.org>]

jsancio commented on a change in pull request #9539:
URL: https://github.com/apache/kafka/pull/9539#discussion_r529572270



##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -370,14 +370,23 @@ private void onBecomeLeader(long currentTimeMs) {
         );
     }
 
+    private List<Voter> getVotersFromVoterId(Set<Integer> voterIds) {

Review comment:
       nit: we can make this a `static` function and rename it to something like `convertToVoters`.

##########
File path: clients/src/main/resources/common/message/LeaderChangeMessage.json
##########
@@ -22,7 +22,9 @@
     {"name": "LeaderId", "type": "int32", "versions": "0+",
       "about": "The ID of the newly elected leader"},
     {"name": "Voters", "type": "[]Voter", "versions": "0+",
-      "about": "The voters who voted for the current leader"}
+      "about": "The voters who voted at the time of election"},

Review comment:
       Based on the implementation on this PR, this field contains all of the voters in the quorum irrespective of whether they voted or not. In other words I think we should change the description to something like "The set of voters in the quorum for this epoch". 

##########
File path: raft/src/main/java/org/apache/kafka/raft/LeaderState.java
##########
@@ -51,6 +53,7 @@ protected LeaderState(
             boolean hasEndorsedLeader = voterId == localId;

Review comment:
       I think we have two options here. This class can contain either:
   ```java
   private final Map<Integer, VoterState> voterReplicaStates = new HashMap<>(); 
   ```
   where we change this expression to `boolean hasEndorsedLeader = grantingVoters.contains(voterId);`
   and remove the field `private final Set<Integer> grantingVoters = new HashSet<>();`
   
   or:
   
   ```java
   private final Set<Integer> voters = new HashSet<>(); 
   private final Set<Integer> grantingVoters = new HashSet<>(); 
   ```
   and change a few of the methods here use these two sets instead of `voterReplicaStates`.
   
   I like the first option but it is up to you.

##########
File path: clients/src/main/resources/common/message/LeaderChangeMessage.json
##########
@@ -22,7 +22,9 @@
     {"name": "LeaderId", "type": "int32", "versions": "0+",
       "about": "The ID of the newly elected leader"},
     {"name": "Voters", "type": "[]Voter", "versions": "0+",
-      "about": "The voters who voted for the current leader"}
+      "about": "The voters who voted at the time of election"},
+    {"name": "GrantingVoters", "type": "[]Voter", "versions": "0+",
+      "about": "The voters who voted for the leader at the time of election"}

Review comment:
       I think we should add an `LeaderEpoch` field that documents the epoch associated with this `LeaderChangeMessage`. What do you think @vamossagar12 and @hachikuji ?




----------------------------------------------------------------
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