You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/08/03 13:28:04 UTC
[GitHub] [ratis] adoroszlai opened a new pull request, #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
adoroszlai opened a new pull request, #702:
URL: https://github.com/apache/ratis/pull/702
## What changes were proposed in this pull request?
`RaftServerImpl#appendEntriesAsync` is still using the common pool here:
https://github.com/apache/ratis/blob/3db8f9eac82efedd51322a0d213300c9a0940bb6/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java#L1416-L1417
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #702:
URL: https://github.com/apache/ratis/pull/702#discussion_r936916794
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1413,7 +1413,7 @@ leaderId, getMemberId(), currentTerm, followerCommit, state.getNextIndex(), NOT_
getRaftServer().getPeer());
}
}
- return JavaUtils.allOf(futures).whenCompleteAsync(
+ return JavaUtils.allOf(futures).whenComplete(
Review Comment:
> 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 -> {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #702:
URL: https://github.com/apache/ratis/pull/702#issuecomment-1206454041
Tested `TestInstallSnapshotNotificationWithGrpc` repeatedly both without and with this patch.
1. [without the patch](https://github.com/adoroszlai/incubator-ratis/actions/runs/2803202957)
* timeout: 1%
* `IllegalArgumentException: ...-SegmentedRaftLog is expected to be opened but it is CLOSED`: 10%
2. [with the patch](https://github.com/adoroszlai/incubator-ratis/actions/runs/2803208217)
* 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`: https://github.com/adoroszlai/incubator-ratis/commit/374396d6b6547f479371435f97bb7b0163bd0d77
With that additional change
* `testInstallSnapshotDuringBootstrap` [timed out](https://github.com/adoroszlai/incubator-ratis/runs/7691221109?check_suite_focus=true#step:5:1117) 1%
* `testInstallSnapshotInstalledEvent` [timed out](https://github.com/adoroszlai/incubator-ratis/runs/7691221280?check_suite_focus=true#step:5:11127) 1%
* `testInstallSnapshotInstalledEvent` [failed](https://github.com/adoroszlai/incubator-ratis/runs/7691221372?check_suite_focus=true#step:5:3617) 1%
* `testRestartFollower` [failed](https://github.com/adoroszlai/incubator-ratis/runs/7691221280?check_suite_focus=true#step:5:10492) 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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #702:
URL: https://github.com/apache/ratis/pull/702#issuecomment-1206697706
`TestInstallSnapshotNotificationWithGrpc` passed in 100/100 runs with f642b14:
https://github.com/adoroszlai/incubator-ratis/actions/runs/2804703023
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #702:
URL: https://github.com/apache/ratis/pull/702#issuecomment-1207270942
@adoroszlai , thanks a lot for working hard on this! How about we merge the current change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #702:
URL: https://github.com/apache/ratis/pull/702#issuecomment-1207340947
Thanks @szetszwo for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai merged pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai merged PR #702:
URL: https://github.com/apache/ratis/pull/702
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on a diff in pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #702:
URL: https://github.com/apache/ratis/pull/702#discussion_r937133081
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1413,7 +1413,7 @@ leaderId, getMemberId(), currentTerm, followerCommit, state.getNextIndex(), NOT_
getRaftServer().getPeer());
}
}
- return JavaUtils.allOf(futures).whenCompleteAsync(
+ return JavaUtils.allOf(futures).whenComplete(
Review Comment:
`TestInstallSnapshotNotificationWithGrpc` may be flaky after all, though I haven't found recent commits with the same failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on a diff in pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #702:
URL: https://github.com/apache/ratis/pull/702#discussion_r937094874
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1413,7 +1413,7 @@ leaderId, getMemberId(), currentTerm, followerCommit, state.getNextIndex(), NOT_
getRaftServer().getPeer());
}
}
- return JavaUtils.allOf(futures).whenCompleteAsync(
+ return JavaUtils.allOf(futures).whenComplete(
Review Comment:
Thanks @szetszwo for the review. [Passing `serverExecutor` was my first attempt](https://github.com/adoroszlai/incubator-ratis/commit/601fb05cb6a32ba380e0782c9d8fe17b1651de43), but it caused [timeout in `TestInstallSnapshotNotificationWithGrpc`](https://github.com/adoroszlai/incubator-ratis/runs/7650650577?check_suite_focus=true#step:5:2830).
It may indicate that the test or some other part of Ratis needs further tweak. I'll take another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] adoroszlai commented on a diff in pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #702:
URL: https://github.com/apache/ratis/pull/702#discussion_r937094874
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1413,7 +1413,7 @@ leaderId, getMemberId(), currentTerm, followerCommit, state.getNextIndex(), NOT_
getRaftServer().getPeer());
}
}
- return JavaUtils.allOf(futures).whenCompleteAsync(
+ return JavaUtils.allOf(futures).whenComplete(
Review Comment:
Thanks @szetszwo for the review. Passing `serverExecutor` was my first attempt, but it caused timeout in `TestInstallSnapshotNotificationWithGrpc`:
https://github.com/adoroszlai/incubator-ratis/runs/7650650577?check_suite_focus=true#step:5:2830
It may indicate that the test or some other part of Ratis needs further tweak. I'll take another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org