[GitHub] [kafka] guozhangwang commented on a change in pull request #11449: MINOR: Log client disconnect events at INFO level

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

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



##########
File path: clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
##########
@@ -319,23 +319,29 @@ public void disconnect(String nodeId) {
         if (connectionStates.isDisconnected(nodeId))
             return;
 
+        log.info("Client requested disconnect from node {} (invoking callbacks for in-flight requests)", nodeId);

Review comment:
       It seems we only have two types of callers for this: 1) admin-client proactively disconnecting, where the caller always log an info explaining why, 2) consumer-client async disconnect the coordinator.
   
   If our target is primarily covering 2) here, what about: 1) make this function `disconnect(nodeId, reason)` where `reason` is a formatted string; 2) rename the `pendingDisconnects` (which is only used for coordinator disconnects) as `pendingCoordinatorDisconnects` and pass in a reason like "since the node was previously recognized as the group coordinator but now it become unknown".

##########
File path: clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
##########
@@ -923,7 +930,7 @@ private void handleApiVersionsResponse(List<ClientResponse> responses,
     private void handleDisconnections(List<ClientResponse> responses, long now) {
         for (Map.Entry<String, ChannelState> entry : this.selector.disconnected().entrySet()) {
             String node = entry.getKey();
-            log.debug("Node {} disconnected.", node);
+            log.info("Node {} disconnected.", node);

Review comment:
       It makes me thinking: why not consolidating `disconnect` to call `processDisconnections` but have two call paths here?

##########
File path: clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
##########
@@ -355,6 +361,7 @@ private void cancelInFlightRequests(String nodeId, long now, Collection<ClientRe
      */
     @Override
     public void close(String nodeId) {
+        log.info("Client requested disconnect from node {} (not invoking callbacks for in-flight requests)", nodeId);

Review comment:
       nit: request closing the socket from node?




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