[GitHub] [kafka] rajinisivaram commented on a change in pull request #10649: KAFKA-12762: Use connection timeout when polling the network for new …

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

rajinisivaram commented on a change in pull request #10649:
URL: https://github.com/apache/kafka/pull/10649#discussion_r662986120



##########
File path: clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java
##########
@@ -105,11 +105,13 @@ public boolean isBlackedOut(String id, long now) {
     public long connectionDelay(String id, long now) {
         NodeConnectionState state = nodeState.get(id);
         if (state == null) return 0;

Review comment:
       The javadoc still seems confusing to me. It seems fine for `disconnected` state because we are returning remaining backoff. For `connecting` state, we are now returning setup timeout, isn't that to cover stalled state? And for `connected` state, we are relying on wakeup based on other events, so this method doesn't actually `handle slow/stalled connections`?

##########
File path: clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java
##########
@@ -273,6 +276,7 @@ public void authenticationFailed(String id, long now, AuthenticationException ex
         nodeState.state = ConnectionState.AUTHENTICATION_FAILED;
         nodeState.lastConnectAttemptMs = now;
         updateReconnectBackoff(nodeState);
+        connectingNodes.remove(id);

Review comment:
       Can we get here with id in `connectingNodes`?




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