RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl

[adoroszlai]

What changes were proposed in this pull request?

RaftServerImpl#appendEntriesAsync is still using the common pool here:

ratis/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java

Lines 1416 to 1417 in 3db8f9e

	return JavaUtils.allOf(futures).whenCompleteAsync(
	(r, t) -> followerState.ifPresent(fs -> fs.updateLastRpcTime(FollowerState.UpdateType.APPEND_COMPLETE))

Preceding steps in appendEntriesAsync are already running on serverExecutor, so I think there is no need to further defer this step.

https://issues.apache.org/jira/browse/RATIS-1656

How was this patch tested?

Regular CI:
https://github.com/adoroszlai/incubator-ratis/actions/runs/2789205160

[szetszwo]

@adoroszlai , thanks a lot for catching this! Please see the comment inlined.

[szetszwo]

Actions supplied for dependent completions of non-async methods may be performed by the thread that completes the current CompletableFuture, or by any other caller of a completion method.

According to the above javadoc https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html , it uses the thread, which is SegmentedRaftLogWorker in this case, completing futures passed to allOf(..) but not the thread running appendEntriesAsync. Therefore, we should pass serverExecutor as below

    return JavaUtils.allOf(futures).whenCompleteAsync(
        (r, t) -> followerState.ifPresent(fs -> fs.updateLastRpcTime(FollowerState.UpdateType.APPEND_COMPLETE)),
        serverExecutor
    ).thenApply(v -> {

[adoroszlai]

Thanks @szetszwo for the review. Passing serverExecutor was my first attempt, but it caused timeout in TestInstallSnapshotNotificationWithGrpc.

It may indicate that the test or some other part of Ratis needs further tweak. I'll take another look.

[adoroszlai]

TestInstallSnapshotNotificationWithGrpc may be flaky after all, though I haven't found recent commits with the same failure.

[szetszwo]

+1 the change looks good.

[adoroszlai]

Tested TestInstallSnapshotNotificationWithGrpc repeatedly both without and with this patch.

1. without the patch
   • timeout: 1%
   • IllegalArgumentException: ...-SegmentedRaftLog is expected to be opened but it is CLOSED: 10%
2. with the patch
   • timeout: 8%
   • IllegalArgumentException: ...-SegmentedRaftLog is expected to be opened but it is CLOSED: none

Timeout happens while cluster is shutting down:

TestTimedOutException: test timed out after 100 seconds
	at org.apache.ratis.server.impl.RaftServerProxy$ImplMap.toString(RaftServerProxy.java:159)
	at java.lang.String.valueOf(String.java:2994)
	at java.lang.StringBuilder.append(StringBuilder.java:136)
	at org.apache.ratis.server.impl.RaftServerProxy.toString(RaftServerProxy.java:637)
	...
	at org.apache.ratis.server.impl.MiniRaftCluster.printServers(MiniRaftCluster.java:534)
	at org.apache.ratis.server.impl.MiniRaftCluster.shutdown(MiniRaftCluster.java:832)
	at org.apache.ratis.server.impl.MiniRaftCluster$Factory$Get.runWithNewCluster(MiniRaftCluster.java:144)
	at org.apache.ratis.server.impl.MiniRaftCluster$Factory$Get.runWithNewCluster(MiniRaftCluster.java:118)
	at org.apache.ratis.InstallSnapshotNotificationTests.testInstallSnapshotDuringBootstrap(InstallSnapshotNotificationTests.java:501)

Looking into that I've found parallelStream in RaftServerProxy: adoroszlai@374396d

With that additional change

• testInstallSnapshotDuringBootstrap timed out 1%
• testInstallSnapshotInstalledEvent timed out 1%
• testInstallSnapshotInstalledEvent failed 1%
• testRestartFollower failed with IllegalArgumentException 1%

TestTimedOutException: test timed out after 100 seconds
	...
	at org.apache.ratis.grpc.client.GrpcClientProtocolClient.setConfiguration(GrpcClientProtocolClient.java:200)
	at org.apache.ratis.grpc.client.GrpcClientRpc.sendRequest(GrpcClientRpc.java:102)
	at org.apache.ratis.client.impl.BlockingImpl.sendRequest(BlockingImpl.java:134)
	at org.apache.ratis.client.impl.BlockingImpl.sendRequestWithRetry(BlockingImpl.java:99)
	at org.apache.ratis.client.impl.AdminImpl.setConfiguration(AdminImpl.java:46)
	at org.apache.ratis.client.api.AdminApi.setConfiguration(AdminApi.java:51)
	at org.apache.ratis.client.api.AdminApi.setConfiguration(AdminApi.java:45)
	at org.apache.ratis.server.impl.MiniRaftCluster.setConfiguration(MiniRaftCluster.java:816)
	at org.apache.ratis.InstallSnapshotNotificationTests.testInstallSnapshotInstalledEvent(InstallSnapshotNotificationTests.java:463)

[adoroszlai]

TestInstallSnapshotNotificationWithGrpc passed in 100/100 runs with f642b14:
https://github.com/adoroszlai/incubator-ratis/actions/runs/2804703023

[szetszwo]

@adoroszlai , thanks a lot for working hard on this! How about we merge the current change?

[adoroszlai]

Thanks @szetszwo for the review.