Copilot commented on code in PR #10747:
URL: https://github.com/apache/ozone/pull/10747#discussion_r3576298659
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java:
##########
@@ -267,39 +265,25 @@ public void close() {
return;
}
+ // Use shutdownNow() (not the graceful shutdown()) so in-flight RPCs are
+ // cancelled and the channel terminates immediately. close() is frequently
+ // invoked from cache eviction while the XceiverClientManager clientCache
+ // monitor is held (HDDS-15849); a blocking graceful drain there serializes
+ // every concurrent acquireClient()/releaseClient() call.
Review Comment:
close() now uses shutdownNow() to cancel in-flight RPCs, but the method
Javadoc above still claims it "waits to finish all ongoing communication". That
description is no longer accurate and could mislead callers about semantics
during shutdown.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java:
##########
@@ -267,39 +265,25 @@ public void close() {
return;
}
+ // Use shutdownNow() (not the graceful shutdown()) so in-flight RPCs are
+ // cancelled and the channel terminates immediately. close() is frequently
+ // invoked from cache eviction while the XceiverClientManager clientCache
+ // monitor is held (HDDS-15849); a blocking graceful drain there serializes
+ // every concurrent acquireClient()/releaseClient() call.
for (ChannelInfo channelInfo : dnChannelInfoMap.values()) {
- channelInfo.getChannel().shutdown();
- }
-
- final long maxWaitNanos =
TimeUnit.SECONDS.toNanos(SHUTDOWN_WAIT_MAX_SECONDS);
- long deadline = System.nanoTime() + maxWaitNanos;
- List<ManagedChannel> nonTerminatedChannels = dnChannelInfoMap.values()
- .stream()
- .map(ChannelInfo::getChannel)
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
-
- while (!nonTerminatedChannels.isEmpty() && System.nanoTime() < deadline) {
- nonTerminatedChannels.removeIf(ManagedChannel::isTerminated);
+ ManagedChannel channel = channelInfo.getChannel();
+ channel.shutdownNow();
try {
- Thread.sleep(SHUTDOWN_WAIT_INTERVAL_MILLIS);
+ if (!channel.awaitTermination(SHUTDOWN_WAIT_MAX_SECONDS,
TimeUnit.SECONDS)) {
+ LOG.warn("Channel {} did not terminate within {}s.", channel,
SHUTDOWN_WAIT_MAX_SECONDS);
+ }
} catch (InterruptedException e) {
- LOG.error("Interrupted while waiting for channels to terminate", e);
+ LOG.error("Interrupted while waiting for channel termination", e);
Thread.currentThread().interrupt();
break;
}
}
Review Comment:
close() now awaits termination for up to SHUTDOWN_WAIT_MAX_SECONDS **per
channel**. With multiple entries in dnChannelInfoMap, this can block for up to
N * timeout, potentially reintroducing long lock holds in the eviction path.
The prior logic enforced a single overall deadline for all channels; consider
keeping the total wait bounded to SHUTDOWN_WAIT_MAX_SECONDS across all channels.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
