MINOR: Improve confusing admin client shutdown logging#10107
MINOR: Improve confusing admin client shutdown logging#10107hachikuji merged 2 commits intoapache:trunkfrom
Conversation
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for this patch. I had seen this weird error message many times :)
one small question is left but it does not obstruct this patch from merging.
| return curNode; | ||
| } | ||
|
|
||
| void abortAndFail(Throwable throwable) { |
There was a problem hiding this comment.
How about changing the input type from Throwable to TimeoutException? The exception passed to this method should NOT be wrapped by TimeoutException. Using explicit type (TimeoutException) can prevent us from passing incorrect exception in the future.
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
Left a minor comment. LGTM otherwise.
| thread.join(waitTimeMs); | ||
| } | ||
| log.debug("Kafka admin client closed."); | ||
| log.info("Kafka admin client closed."); |
There was a problem hiding this comment.
I wonder if changing this log level is really necessary. What's the rational behind it?
There was a problem hiding this comment.
I think it is helpful to get at least one log message at info level in shutdown. Really at the start of shutdown is best since it triggers logic which can be difficult to explain if you don't know shutdown is taking place. Anyway, let me remove this change here and I can open a separate patch so that we can do this consistently in all the clients.
…e-allocations-lz4 * apache-github/trunk: (118 commits) KAFKA-12327: Remove MethodHandle usage in CompressionType (apache#10123) KAFKA-12297: Make MockProducer return RecordMetadata with values as per contract MINOR: Update zstd and use classes with no finalizers (apache#10120) KAFKA-12326: Corrected regresion in MirrorMaker 2 executable introduced with KAFKA-10021 (apache#10122) KAFKA-12321 the comparison function for uuid type should be 'equals' rather than '==' (apache#10098) MINOR: Add FetchSnapshot API doc in KafkaRaftClient (apache#10097) MINOR: KIP-631 KafkaConfig fixes and improvements (apache#10114) KAFKA-12272: Fix commit-interval metrics (apache#10102) MINOR: Improve confusing admin client shutdown logging (apache#10107) MINOR: Add BrokerMetadataListener (apache#10111) MINOR: Support Raft-based metadata quorums in system tests (apache#10093) MINOR: add the MetaLogListener, LocalLogManager, and Controller interface. (apache#10106) MINOR: Introduce the KIP-500 Broker lifecycle manager (apache#10095) MINOR: Remove always-passing validation in TestRecordTest#testProducerRecord (apache#9930) KAFKA-5235: GetOffsetShell: Support for multiple topics and consumer configuration override (KIP-635) (apache#9430) MINOR: Prevent creating partition.metadata until ID can be written (apache#10041) MINOR: Add RaftReplicaManager (apache#10069) MINOR: Add ClientQuotaMetadataManager for processing QuotaRecord (apache#10101) MINOR: Rename DecommissionBrokers to UnregisterBrokers (apache#10084) MINOR: KafkaBroker.brokerState should be volatile instead of AtomicReference (apache#10080) ... clients/src/main/java/org/apache/kafka/common/record/CompressionType.java core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala
3 participants
If the admin client is shutdown with some unfinished calls, we see messages such as the following in the log:
The problem is that we are using passing
Long.MaxValueas the current time inCall.failin order to ensure the call is timed out and we are discarding the original cause. The patch fixes the problem by settingaborted=trueinstead and preserving the original exception message.Committer Checklist (excluded from commit message)