HDDS-5537. Limit grpc threads in ReplicationServer. #2498

ChenSammi · 2021-08-04T13:19:28Z

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

jojochuang

Thanks for posting the PR. I did realize this is a potential problem but never got the chance to implement a fix.

...rvice/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationServer.java

jojochuang

i don't like how the test is written but as long as it does fail when there's an error, I am fine.

ChenSammi · 2021-08-09T07:38:00Z

i don't like how the test is written but as long as it does fail when there's an error, I am fine.

The test tries to verify that when epoll is not available, the eventLoopGroup still can be created. Previously this part of logic is handled by grpc itself. Just want to make sure it doesn't break anything.

ChenSammi · 2021-08-09T07:40:20Z

Hey @elek , would you help to take another look of this patch ?

adoroszlai

Thanks @ChenSammi for the patch. I only have a question and a minor code suggestion.

...r-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java

adoroszlai · 2021-09-16T19:23:50Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java

Can you please explain why setting channel type and event loop group are needed? Based on NettyServerBuilder#channelType javadoc this is the same as the default behavior:

Specifies the channel type to use, by default we use {@code EpollServerSocketChannel} if available, otherwise using {@link NioServerSocketChannel}.

@adoroszlai , thanks for the code review. It's because EpollServerSocketChannel is not supported on Mac. I believe most of us use Mac for code develoepment. I'd like to make sure replication related unit tests will not fail on Mac book. You can find that there is an channelType identify process ahead.

we can consider adding KQueueSocketChannel support for Mac later.

jojochuang · 2021-10-21T08:47:53Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java

How is this different from EpollServerSocketChannel?
If we use domain socket do we bind to a domain socket instead like the example in here? https://stackoverflow.com/questions/48354562/binding-a-java-grpc-server-to-a-unix-domain-socket/48358536#48358536

adoroszlai · 2021-10-21T19:00:05Z

@ChenSammi it seems unit test failures are related to this change, the same failures repeated twice:

Error:    TestDatanodeStateMachine.testDatanodeStateContext:262 » Timeout Timed out wait...
Error:    TestStorageVolumeChecker.testVolumeDeletion:231 » OutOfMemory unable to create...
Error:    TestDatanodeUpgradeToScmHA.testReadsDuringFinalization:138->addContainer:612->dispatchRequest:672->dispatchRequest:679 » NullPointer

https://github.com/apache/ozone/runs/3933857760?check_suite_focus=true#step:4:1471
https://github.com/apache/ozone/runs/3967634877?check_suite_focus=true#step:4:1461

adoroszlai · 2021-11-08T16:18:04Z

This one turned out to be unrelated, fixed by #2806:

Error:    TestStorageVolumeChecker.testVolumeDeletion:231 » OutOfMemory unable to create...

But these two are still there:

Error:    TestDatanodeStateMachine.testDatanodeStateContext:262 » Timeout Timed out wait...
Error:    TestDatanodeUpgradeToScmHA.testReadsDuringFinalization:138->addContainer:612->dispatchRequest:672->dispatchRequest:679 » NullPointer

https://github.com/apache/ozone/runs/4139083271#step:4:1409

ChenSammi · 2021-11-09T02:43:28Z

This one turned out to be unrelated, fixed by #2806:
Error:    TestStorageVolumeChecker.testVolumeDeletion:231 » OutOfMemory unable to create...
But these two are still there:
Error:    TestDatanodeStateMachine.testDatanodeStateContext:262 » Timeout Timed out wait...
Error:    TestDatanodeUpgradeToScmHA.testReadsDuringFinalization:138->addContainer:612->dispatchRequest:672->dispatchRequest:679 » NullPointer
https://github.com/apache/ozone/runs/4139083271#step:4:1409

Thanks @adoroszlai ， will look at it.

…ozone/container/replication/ReplicationServer.java Co-authored-by: Doroszlai, Attila <[email protected]>

adoroszlai · 2021-12-22T14:43:41Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java

+    Class<? extends ServerChannel> channelType;
+
+    if (Epoll.isAvailable()) {
+      eventLoopGroup = new EpollEventLoopGroup(poolSize / 4);


Can you please explain how 4 is derived? (Magic number?)

adoroszlai · 2021-12-22T14:43:46Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java

+      eventLoopGroup.shutdownGracefully().sync();
+      executor.shutdown();
+      executor.awaitTermination(5L, TimeUnit.SECONDS);
+      server.shutdown().awaitTermination(5L, TimeUnit.SECONDS);


Event loop group should be shutdown last (#2900 fixed similar problem in XceiverServerGrpc, introduced in #2824):

Suggested change

eventLoopGroup.shutdownGracefully().sync();

executor.shutdown();

executor.awaitTermination(5L, TimeUnit.SECONDS);

server.shutdown().awaitTermination(5L, TimeUnit.SECONDS);

executor.shutdown();

executor.awaitTermination(5L, TimeUnit.SECONDS);

server.shutdown().awaitTermination(5L, TimeUnit.SECONDS);

eventLoopGroup.shutdownGracefully().sync();

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java

adoroszlai · 2022-03-09T20:27:33Z

/pending

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

/pending

github-actions · 2022-03-31T00:13:31Z

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

jojochuang reviewed Aug 6, 2021

View reviewed changes

jojochuang requested changes Aug 6, 2021

View reviewed changes

...rvice/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationServer.java Outdated Show resolved Hide resolved

...rvice/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationServer.java Outdated Show resolved Hide resolved

ChenSammi requested a review from jojochuang August 9, 2021 02:21

jojochuang approved these changes Aug 9, 2021

View reviewed changes

adoroszlai reviewed Sep 16, 2021

View reviewed changes

ChenSammi force-pushed the HDDS-5537 branch from 94378b9 to 367b164 Compare October 19, 2021 01:37

jojochuang reviewed Oct 21, 2021

View reviewed changes

ChenSammi and others added 8 commits November 10, 2021 15:51

HDDS-5537. Limit grpc threads in ReplicationServer.

344f939

Improve code

209ddd3

fix checkstyle

ebf43e4

refine UT

f937d40

change the default queue size and exectuor size

d0f4a39

Update hadoop-hdds/container-service/src/main/java/org/apache/hadoop/…

8a51707

…ozone/container/replication/ReplicationServer.java Co-authored-by: Doroszlai, Attila <[email protected]>

add import

deb7f02

change ELG type to EpollServerSocketChannel

e0f5d38

ChenSammi force-pushed the HDDS-5537 branch from 3a06ea6 to e0f5d38 Compare November 10, 2021 07:51

ChenSammi and others added 2 commits November 10, 2021 16:48

fix failed TestDatanodeConfiguration

0c9c5b1

Merge remote-tracking branch 'origin/master' into HDDS-5537

cca1510

adoroszlai requested changes Dec 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into HDDS-5537

87d7656

github-actions bot added the pending label Mar 9, 2022

github-actions bot requested changes Mar 9, 2022

View reviewed changes

github-actions bot closed this Mar 31, 2022

HDDS-5537. Limit grpc threads in ReplicationServer. #2498

HDDS-5537. Limit grpc threads in ReplicationServer. #2498

Uh oh!

Conversation

ChenSammi commented Aug 4, 2021

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Aug 9, 2021

Uh oh!

ChenSammi commented Aug 9, 2021

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

ChenSammi Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

jojochuang Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

jojochuang Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Oct 21, 2021

Uh oh!

adoroszlai commented Nov 8, 2021

Uh oh!

ChenSammi commented Nov 9, 2021

Uh oh!

adoroszlai Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

adoroszlai Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai commented Mar 9, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants