[GitHub] [kafka] guozhangwang commented on a diff in pull request #13190: KAFKA-12639: exit upon expired timer to prevent tight looping

["guozhangwang (via GitHub)" <gi...@apache.org>]

guozhangwang commented on code in PR #13190:
URL: https://github.com/apache/kafka/pull/13190#discussion_r1096112947


##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -501,13 +501,16 @@ boolean joinGroupIfNeeded(final Timer timer) {
                 }
 
                 if (exception instanceof UnknownMemberIdException ||
-                    exception instanceof IllegalGenerationException ||
-                    exception instanceof RebalanceInProgressException ||
-                    exception instanceof MemberIdRequiredException)
+                        exception instanceof IllegalGenerationException ||
+                        exception instanceof RebalanceInProgressException ||
+                        exception instanceof MemberIdRequiredException)
                     continue;
                 else if (!future.isRetriable())
                     throw exception;
 
+                if (timer.isExpired()) {

Review Comment:
   Could you add a couple comments here explaining why we check the timer again here in addition to in line 452 above? Maybe something like this:
   
   ```
   We check the timer again after calling poll with the timer since it's possible that even after the timer has elapsed, the next client.poll(timer) would immediately return an error response which would cause us to not exiting the while loop.
   ```



##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java:
##########
@@ -501,13 +501,16 @@ boolean joinGroupIfNeeded(final Timer timer) {
                 }
 
                 if (exception instanceof UnknownMemberIdException ||
-                    exception instanceof IllegalGenerationException ||
-                    exception instanceof RebalanceInProgressException ||
-                    exception instanceof MemberIdRequiredException)
+                        exception instanceof IllegalGenerationException ||
+                        exception instanceof RebalanceInProgressException ||
+                        exception instanceof MemberIdRequiredException)

Review Comment:
   Should we actually do the timer check before this? Since otherwise if the exception from the immediately returned responses is any of those four, we would still `continue` and skip the check below.
   
   More concretely I think we can just move the remaining logic inside the `if` call:
   
   ```
   if (!future.isRetriable()) {
       throw ..
   } else {
       if (timer.isExpired()) {
           return false;
       } else if (exception instance of..) {
           continue;
       } else {
           timer.sleep(..)
       }
   }
   ```



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