Copilot commented on code in PR #22529:
URL: https://github.com/apache/kafka/pull/22529#discussion_r3387731409
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -5166,6 +5166,15 @@ void maybeRetry(long currentTimeMs, Throwable throwable)
{
@Override
boolean handleNodeUnavailable(long currentTimeMs) {
+ // Don't intervene while the client is shutting down.
Re-running the lookup enqueues
+ // new calls via runnable.call(), which are rejected during
close ("Cannot accept new
+ // calls when AdminClient is closing"). Leaving the call in
pendingCalls preserves the
+ // normal close(timeout) handling, so it can still be retried
(or assigned, should the
+ // broker reappear) within the shutdown grace period instead
of failing immediately.
+ if (hardShutdownTimeMs.get() != INVALID_SHUTDOWN_TIME) {
+ return false;
+ }
Review Comment:
The new shutdown guard (`hardShutdownTimeMs != INVALID_SHUTDOWN_TIME`)
changes retry behavior specifically during `close(timeout)` (skipping the
stale-leader lookup recovery). There doesn't appear to be a unit/integration
test that exercises the stale-cached-leader recovery path while a
`close(Duration)` is in progress, so this behavior could regress without
detection. Consider adding a test that seeds the partition-leader cache with a
broker that is removed from metadata, initiates a request, calls
`close(Duration)` with a non-zero grace period, and asserts the call is not
failed immediately with `IllegalStateException("Cannot accept new calls when
AdminClient is closing.")` (it should instead follow the normal close/timeout
path).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
