HDDS-15849. Force channel shutdown in XceiverClientGrpc.close#10747
Open
yandrey321 wants to merge 2 commits into
Open
HDDS-15849. Force channel shutdown in XceiverClientGrpc.close#10747yandrey321 wants to merge 2 commits into
yandrey321 wants to merge 2 commits into
Conversation
Contributor
|
cc @ptlrs |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses the HDDS-15849 performance regression by removing the fixed 100ms polling sleep from XceiverClientGrpc.close() and adding a regression test that detects close-time contention caused by eviction-driven shutdown under the XceiverClientManager cache monitor.
Changes:
- Switch
XceiverClientGrpc.close()from graceful shutdown + sleep polling toshutdownNow()+awaitTermination()to avoid the mandatory 100ms floor per close. - Add
TestXceiverClientManagerCloseContentionto assert eviction-driven closes do not serialize concurrent acquire/release activity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java | Reworks gRPC channel shutdown behavior in close() to eliminate the fixed sleep-based termination polling. |
| hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverClientManagerCloseContention.java | Adds a regression test that detects cache-eviction close contention and asserts acceptable per-close wall-clock cost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
273
to
285
| 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; | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
its an old behavior that was supposed to be addressed by HDDS-14571, I'll file a new JIRA to get the better fix that does not introduce this regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
XceiverClientGrpc.close() performs a blocking graceful channel shutdown, and because clients are torn down through XceiverClientManager's cache eviction, that blocking wait runs while the clientCache monitor is held — serializing every concurrent acquireClient() / releaseClient() call.
HDDS-14571 changed close() from a forced shutdownNow() (which cancels in-flight RPCs and terminates in ~1 ms) to a graceful shutdown() followed by a polling loop that always sleeps at least SHUTDOWN_WAIT_INTERVAL_MILLIS (100 ms) before it can observe termination:
Every close() now costs ≥100 ms instead of ~1 ms. The eviction path that invokes it holds the cache monitor throughout:
So concurrent readers that create/evict short-lived clients (e.g. EC read/checksum collection, which uses a distinct client per placement group) serialize behind a ≥100 ms lock hold per closed channel and degrade non-linearly with concurrency.
Fix: restore immediate termination in close() — use channel.shutdownNow() followed by channel.awaitTermination(SHUTDOWN_WAIT_MAX_SECONDS, ...), and drop the mandatory-sleep polling loop. This keeps HDDS-14571's goal (no synchronized methods on XceiverClientGrpc) while eliminating the blocking wait under the cache lock.
Co authored with Claude Code
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15849
How was this patch tested?
Added TestXceiverClientManagerCloseContention, a unit test that exercises the real XceiverClientManager and XceiverClientGrpc.close() (real lazily-connected plaintext gRPC channels to random local datanode addresses; no MiniOzoneCluster and no live datanode required, since close() only shuts channels down). maxCacheSize=1 plus a distinct pipeline per iteration forces an eviction-driven close on nearly every acquisition; 8 threads × 8 cycles produce 63 closes. It measures average wall-clock per close and asserts it stays well under the pre-fix ~100 ms floor.
The same test, unmodified, against the code before and after this change:
~34× faster per close