kevin-wu24 commented on code in PR #20859:
URL: https://github.com/apache/kafka/pull/20859#discussion_r2561465070


##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -2335,8 +2344,11 @@ private boolean handleAddVoterResponse(
         /* These error codes indicate the replica was successfully added or 
the leader is unable to
          * process the request. In either case, reset the update voter set 
timer to back off.
          */
-        if (error == Errors.NONE || error == Errors.REQUEST_TIMED_OUT ||
-            error == Errors.DUPLICATE_VOTER) {
+        if (error == Errors.NONE) {
+            
quorum.followerStateOrThrow().resetUpdateVoterSetPeriod(currentTimeMs);
+            hasAutoJoined = true;

Review Comment:
   Take a look at this code from `AddVoterHandler#handleApiVersionsResponse`:
   ```
   current.setLastOffset(leaderState.appendVotersRecord(newVoters, 
currentTimeMs));
           if (!current.ackWhenCommitted()) { ***we execute the below when 
auto-join is enabled***
               // complete the future to send response, but do not reset the 
state,
               // since the new voter set is not yet committed
               current.future().complete(RaftUtil.addVoterResponse(Errors.NONE, 
null));
           }
   ```
   When auto-join is enabled, we send the RPC response BEFORE the new voters 
record is committed. The motivation behind this is detailed in: 
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1186%3A+Update+AddRaftVoterRequest+RPC+to+support+auto-join,
 and is necessary for a fully available auto-join feature.
   
   What this means here is that our local node may not actually have 
auto-joined, but we set `hasAutoJoined = true`. For example, what if the remote 
leader fails over before the VotersRecord adding the local node is committed? 
The local node has not added itself via auto-join, and will not until it 
restarts, despite the user expecting it too.
   
   The "more correct" place to set `hasAutoJoined = true` is in `pollFollower` 
if we are in the `quorum.isVoter()` case AND `quorumConfig.autoJoin() == true`.



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

Reply via email to