[GitHub] [kafka] dajac commented on a change in pull request #11451: KAFKA-13419: Only reset generation ID when ILLEGAL_GENERATION error

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

dajac commented on a change in pull request #11451:
URL: https://github.com/apache/kafka/pull/11451#discussion_r770737533



##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java
##########
@@ -755,18 +755,17 @@ protected void onJoinPrepare(int generation, String memberId) {
 
     @Override
     public void onLeavePrepare() {
-        // Save the current Generation and use that to get the memberId, as the hb thread can change it at any time
+        // Save the current Generation, as the hb thread can change it at any time
         final Generation currentGeneration = generation();
-        final String memberId = currentGeneration.memberId;
 
-        log.debug("Executing onLeavePrepare with generation {} and memberId {}", currentGeneration, memberId);
+        log.debug("Executing onLeavePrepare with generation {}", currentGeneration);
 
         // we should reset assignment and trigger the callback before leaving group
         Set<TopicPartition> droppedPartitions = new HashSet<>(subscriptions.assignedPartitions());
 
         if (subscriptions.hasAutoAssignedPartitions() && !droppedPartitions.isEmpty()) {
             final Exception e;
-            if (generation() == Generation.NO_GENERATION || rebalanceInProgress()) {
+            if (currentGeneration.equals(Generation.NO_GENERATION) || rebalanceInProgress()) {

Review comment:
       I am not sure about this one. Is it correct to compare to `Generation.NO_GENERATION` here or do we need to compare to the `generationId`?

##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java
##########
@@ -705,13 +705,13 @@ protected void onJoinPrepare(int generation, String memberId) {
         // so that users can still access the previously owned partitions to commit offsets etc.
         Exception exception = null;
         final Set<TopicPartition> revokedPartitions;
-        if (generation == Generation.NO_GENERATION.generationId &&
+        if (generation == Generation.NO_GENERATION.generationId ||

Review comment:
       Is `|| memberId.equals(Generation.NO_GENERATION.memberId)` really necessary? My understanding is that a reset `memberId` implies that `generationId` was also reset. I guess that it does not hurt to have it.




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