New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RATIS-1497. Avoid using ForkJoinPool.commonPool() in GrpcClientProtocolService #587
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @szetszwo for working this.
One question: should the new executors be declared as ExecutorService
and shutdown()
when appropriate?
@@ -63,11 +64,15 @@ | |||
import java.util.Optional; | |||
import java.util.UUID; | |||
import java.util.concurrent.*; | |||
import java.util.function.Predicate; | |||
import java.util.function.Supplier; | |||
import java.util.stream.Collectors; | |||
import java.util.stream.Stream; | |||
|
|||
class RaftServerProxy implements RaftServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May also need EXECUTOR
:
ratis/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java
Lines 554 to 556 in 7d2f0f3
public CompletableFuture<GroupInfoReply> getGroupInfoAsync(GroupInfoRequest request) { | |
return getImplFuture(request.getRaftGroupId()).thenApplyAsync( | |
server -> server.getGroupInfo(request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks.
bd2129f
to
0f83874
Compare
The shutdown() method is to stop accepting new tasks. Yes, we should call shutdown() except for the static PeerProxyMap.EXECUTOR, shutdown() is not needed. |
BTW, I have reverted the LeaderStateImpl change since the GrpcClientProtocolService change will fix the reported problem. LeaderStateImpl will be fixed by https://issues.apache.org/jira/browse/RATIS-1500 ( #588 ) |
Due to the test failures, I keep only the grpc change here and have reverted the server change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @szetszwo for updating the patch. The only failing test TestRaftOutputStreamWithGrpc
is known to be flaky (RATIS-1493).
See https://issues.apache.org/jira/browse/RATIS-1497