[GitHub] [kafka] guozhangwang commented on a change in pull request #9671: KAFKA-10793: move handling of FindCoordinatorFuture to fix race condition

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

guozhangwang commented on a change in pull request #9671:
URL: https://github.com/apache/kafka/pull/9671#discussion_r558623815



##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -1342,10 +1351,17 @@ public void run() {
                         long now = time.milliseconds();
 
                         if (coordinatorUnknown()) {
-                            if (findCoordinatorFuture != null || lookupCoordinator().failed())
+                            if (findCoordinatorFuture != null || lookupCoordinator().failed()) {

Review comment:
       The nested condition is a bit awkward, how about this:
   
   ```
   // try to find coordinator once if we have not yet done so; otherwise backoff properly
   if (findCoordinatorFuture != null) {
       AbstractCoordinator.this.wait(rebalanceConfig.retryBackoffMs);
   } else {
       lookupCoordinator();
   }
   
   // now findCoordinatorFuture should not be null;
   // if it has failed, clear it so that hb thread can try discover again in the next loop in case main thread is busy
   if (findCoordinatorFuture.failed()) {
       clearFindCoordinatorFuture();
   }
   ```




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