Skip to content

Conversation

@ptlrs
Copy link
Contributor

@ptlrs ptlrs commented Feb 6, 2026

What changes were proposed in this pull request?

This change improves the closing mechanism for the Grpc client.
It closes all channels at the same time.
It waits only 5 seconds for closing the channels

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14454

How was this patch tested?

CI: https://github.com/ptlrs/ozone/actions/runs/21736799049

@ptlrs ptlrs force-pushed the HDDS-14454-XceiverClientGrpcClose branch from a2dd999 to af22ced Compare February 6, 2026 01:32
@ptlrs ptlrs marked this pull request as draft February 6, 2026 01:57
try {
channel.awaitTermination(60, TimeUnit.MINUTES);
if (!nonTerminatedChannels.isEmpty()) {
ManagedChannel channel = nonTerminatedChannels.get(0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was due to the presence of synchronized methods and to workaround findbugs error

Run if [[ -s "target/$SCRIPT/summary.md" ]]; then
M M SWL: org.apache.hadoop.hdds.scm.XceiverClientGrpc.close() calls Thread.sleep() with a lock held  At XceiverClientGrpc.java:[line 258]
Error: Process completed with exit code 1.

* to finish ongoing communication, then interrupts the connection anyway.
*/
@Override
public synchronized void close() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with shutdown() instead of shutdownNow() we should not need synchronized here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use concurrent maps here, it should help with removing synchronized methods.

Copy link
Contributor Author

@ptlrs ptlrs Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The follow up PR #9718 addresses methods other than close

ptlrs added 2 commits February 5, 2026 18:41
…cleaner channel termination handling."

This reverts commit 7b6b0ee.
Copy link

@yandrey321 yandrey321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ptlrs ptlrs marked this pull request as ready for review February 6, 2026 19:17
@ptlrs
Copy link
Contributor Author

ptlrs commented Feb 9, 2026

Hi @jojochuang, could you please review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants