HDDS-14430. GrpcOmTransport.shutdown() could wait up to number_of_channels * 5s#9648
HDDS-14430. GrpcOmTransport.shutdown() could wait up to number_of_channels * 5s#9648adoroszlai merged 4 commits intoapache:masterfrom
Conversation
adoroszlai
changed the title
|
Thanks @cchung100m for the patch. There is a checkstyle and a findbugs problem each. (It's best to wait for clean CI run in fork before opening pull request to reduce noise.) |
cchung100m
force-pushed
the
HDDS-14430
branch
from
33997f8 to
b74469f
Compare
|
Hi @adoroszlai |
|
@yandrey321 please take a look |
|
|
||
| while (elapsed < maxWaitMillis) { | ||
| allTerminated = true; | ||
| for (Map.Entry<String, ManagedChannel> entry : channels.entrySet()) { |
There was a problem hiding this comment.
- collect all channels into the list before the while loop
- when channel is terminated remove it from the list (inside while loop)
- the last 'for' loop for warning messages can be replaced with a single log with a list of remaining non-closed channels.
There was a problem hiding this comment.
with this approach allTerminated is not needed and can be replaced with check is list of non terminated channel is empty.
There was a problem hiding this comment.
Hi @yandrey321
Thanks for suggestions and I updated the part you mentioned.
| } catch (Exception e) { | ||
| LOG.error("failed to shutdown OzoneManagerServiceGrpc channel {} : {}", | ||
| entry.getKey(), e); | ||
| Thread.sleep(100); |
| channel.shutdown(); | ||
| } | ||
|
|
||
| final long maxWaitMillis = TimeUnit.SECONDS.toMillis(5); |
…of waiting for each channel sequentially
|
|
||
| private static final String CLIENT_NAME = "GrpcOmTransport"; | ||
| private static final int SHUTDOWN_WAIT_INTERVAL = 100; | ||
| private static final int SHUTDOWN_MAX_WAIT_MILLIS = 5; |
|
|
||
| final long maxWaitMillis = TimeUnit.SECONDS.toMillis(SHUTDOWN_MAX_WAIT_MILLIS); | ||
| long deadline = System.currentTimeMillis() + maxWaitMillis; | ||
| List<Map.Entry<String, ManagedChannel>> nonTerminated = |
There was a problem hiding this comment.
List<ManagedChannel> nonTerminated = channels.values().stream().collect(Collectors.toList());
| } | ||
|
|
||
| final long maxWaitMillis = TimeUnit.SECONDS.toMillis(SHUTDOWN_MAX_WAIT_MILLIS); | ||
| long deadline = System.currentTimeMillis() + maxWaitMillis; |
There was a problem hiding this comment.
use monotonic time: System.nanoTime();
| List<Map.Entry<String, ManagedChannel>> nonTerminated = | ||
| new ArrayList<>(channels.entrySet()); | ||
|
|
||
| while (!nonTerminated.isEmpty() && System.currentTimeMillis() < deadline) { |
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the change and review.
|
Thanks @cchung100m for the patch, @sodonnel, @yandrey321 for the review. |
|
Thanks to @yandrey321 @adoroszlai @sodonnel 😄 |
4 participants
What changes were proposed in this pull request?
Use separate loop with periodic check of channel.isTerminated() with overall limit of 5s.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14430
How was this patch tested?
Existing unit and integration tests