MINOR: Log client disconnect events at INFO level#11449
MINOR: Log client disconnect events at INFO level#11449hachikuji merged 3 commits intoapache:trunkfrom
Conversation
|
The main thing I am interested in here is disconnects when there are inflight requests, so we can probably be more selective about the logging if there are any concerns. |
hachikuji
force-pushed
the
increase-disconnect-logging-verbosity
branch
2 times, most recently
from
34cb715 to
3a46c11
Compare
|
It's worth noting that we already have the WARN log here when a connection cannot be established: https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L766. This is probably the most likely case to be spammy. The other additions from this PR are less common situations such as request timeouts and client-side requests to disconnect. |
hachikuji
force-pushed
the
increase-disconnect-logging-verbosity
branch
from
3a46c11 to
4072760
Compare
| if (connectionStates.isDisconnected(nodeId)) | ||
| return; | ||
|
|
||
| log.info("Client requested disconnect from node {} (invoking callbacks for in-flight requests)", nodeId); |
There was a problem hiding this 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".
There was a problem hiding this comment.
I think it is useful to cover both cases (as well as any future cases). Let me consider adding a disconnect reason.
There was a problem hiding this comment.
I found it a little simpler to add a separate log message for the coordinator case. Let me know if that seems reasonable. In general, I'm not too worried about the verbosity of this message since client-initiated disconnects should be relatively rare (e.g. after session or request timeouts).
| for (Map.Entry<String, ChannelState> entry : this.selector.disconnected().entrySet()) { | ||
| String node = entry.getKey(); | ||
| log.debug("Node {} disconnected.", node); | ||
| log.info("Node {} disconnected.", node); |
There was a problem hiding this comment.
It makes me thinking: why not consolidating disconnect to call processDisconnections but have two call paths here?
There was a problem hiding this comment.
I agree there is room for consolidation, but I was trying to avoid refactoring here. Could we consider this separately?
|
Sounds good. LGTM! |
|
@hachikuji Is it worth cherry-picking this one into 3.1 branch as well? |
…ntegration-11-nov * ak/trunk: (15 commits) KAFKA-13429: ignore bin on new modules (apache#11415) KAFKA-12648: introduce TopologyConfig and TaskConfig for topology-level overrides (apache#11272) KAFKA-12487: Add support for cooperative consumer protocol with sink connectors (apache#10563) MINOR: Log client disconnect events at INFO level (apache#11449) MINOR: Remove topic null check from `TopicIdPartition` and adjust constructor order (apache#11403) KAFKA-13417; Ensure dynamic reconfigurations set old config properly (apache#11448) MINOR: Adding a constant to denote UNKNOWN leader in LeaderAndEpoch (apache#11477) KAFKA-10543: Convert KTable joins to new PAPI (apache#11412) KAFKA-12226: Commit source task offsets without blocking on batch delivery (apache#11323) KAFKA-13396: Allow create topic without partition/replicaFactor (apache#11429) ...
Client disconnects are crucial events for debugging. The fact that we only log them at DEBUG/TRACE means we rarely have them available outside of a testing context. This patch therefore increases verbosity to INFO level. In practice, we already have backoff configurations which should prevent these logs from getting too spammy. Reviewers: Guozhang Wang <wangguoz@gmail.com>
Client disconnects are crucial events for debugging. The fact that we only log them at DEBUG/TRACE means we rarely have them available outside of a testing context. This patch therefore increases verbosity to INFO level. In practice, we already have backoff configurations which should prevent these logs from getting too spammy. Reviewers: Guozhang Wang <wangguoz@gmail.com>
3 participants
Client disconnects are crucial events for debugging. The fact that we only log them at DEBUG/TRACE means we rarely have them available outside of a testing context. This patch therefore increases verbosity to INFO level. In practice, we already have backoff configurations which should prevent these logs from getting too spammy.
Committer Checklist (excluded from commit message)