vamossagar12 commented on code in PR #14455:
URL: https://github.com/apache/kafka/pull/14455#discussion_r1338305899
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -1546,9 +1546,9 @@ void enqueue(Call call, long now) {
*/
void call(Call call, long now) {
if (hardShutdownTimeMs.get() != INVALID_SHUTDOWN_TIME) {
- log.debug("The AdminClient is not accepting new calls. Timing
out {}.", call);
+ log.debug("Cannot accept new call {} when AdminClient is
closing.", call);
call.handleTimeoutFailure(time.milliseconds(),
Review Comment:
I am thinking if we are throwing an `IllegalStateException`, then we don't
need to route it through `handleTimeoutFailure` method. We can directly invoke
`call.handleFailure()` from this point and pass the `IllegalStateException` to
it. WDYT?
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -924,7 +924,7 @@ private void handleTimeoutFailure(long now, Throwable
cause) {
log.debug("{} timed out at {} after {} attempt(s)", this, now,
tries,
new Exception(prettyPrintException(cause)));
}
- if (cause instanceof TimeoutException) {
+ if (cause instanceof TimeoutException || cause instanceof
IllegalStateException) {
Review Comment:
I feel it's slightly confusing to handle `IllegalStateException` within
`handleTimeoutException`. This might not be needed if we do what I suggested in
the other comment.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
