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

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

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



##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -317,6 +317,9 @@ private void appendLeaderChangeMessage(LeaderState state, long currentTimeMs) {
             .map(follower -> new Voter().setVoterId(follower))
             .collect(Collectors.toList());
 
+        // Adding the leader to the voters as the protocol ensures that leader always votes for itself.
+        voters.add(new Voter().setVoterId(state.election().leaderId()));

Review comment:
       Thanks @hachikuji . So, from the context of this PR, do you suggest to continue using the definition of Voters in the LeaderChange message as all the voters ? Maybe we can create separate issues for:
   1) changing LeaderChange message to include both all voters and endorsing voters for that Leader.
   2) Make relevant changes to be able to pass the grantingVoters information from CandidateState to LeaderState. These include the things you mentioned like removing `onBecomeLeader` from `initialize()` or throw exception.
   
   Just curious on the last part. As per KAFKA-10527, a node can't be initialized as a leader. So, this block of code is effectively unused then:
   
   `if (quorum.isLeader()) {
   onBecomeLeader(currentTimeMs);
           } else if (quorum.isCandidate()) {
               onBecomeCandidate(currentTimeMs);
           } `
   
   
   Let me know how should I proceed here...

##########
File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java
##########
@@ -317,6 +317,9 @@ private void appendLeaderChangeMessage(LeaderState state, long currentTimeMs) {
             .map(follower -> new Voter().setVoterId(follower))
             .collect(Collectors.toList());
 
+        // Adding the leader to the voters as the protocol ensures that leader always votes for itself.
+        voters.add(new Voter().setVoterId(state.election().leaderId()));

Review comment:
       Thanks @hachikuji . So, from the context of this PR, do you suggest to continue using the definition of Voters in the LeaderChange message as all the voters ? Maybe we can create separate issues for:
   1) changing LeaderChange message to include both all voters and endorsing voters for that Leader.
   2) Make relevant changes to be able to pass the grantingVoters information from CandidateState to LeaderState. These include the things you mentioned like removing `onBecomeLeader` from `initialize()` or throw exception.
   
   Just curious on the last part. As per KAFKA-10527, a node can't be initialized as a leader. So, this block of code is effectively unused then:
   
   `if (quorum.isLeader()) {
   onBecomeLeader(currentTimeMs);
           } else if (quorum.isCandidate()) {
               onBecomeCandidate(currentTimeMs);
           } `
   
   
   Or should we chase all the above as part of this PR itself? Plz let me know.




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