sanpwc commented on code in PR #4214:
URL: https://github.com/apache/ignite-3/pull/4214#discussion_r1713252052
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -713,11 +714,9 @@ private void scheduleRetry(Runnable runnable) {
* @return {@code True} if this is a recoverable exception.
*/
private static boolean recoverable(Throwable t) {
- if (t instanceof ExecutionException || t instanceof
CompletionException) {
- t = t.getCause();
- }
+ t = unwrapCause(t);
Review Comment:
What's the point in double `unwrapCause` call? Just in case because
recoverable is an utility method?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -598,6 +597,8 @@ private void handleThrowable(
long stopTime,
CompletableFuture<? extends NetworkMessage> fut
) {
+ err = unwrapCause(err);
Review Comment:
Am I right that you will always exclude CompletionException ||
ExecutionException from the stack even if the root cause is not related to
"Raft peer cannot be resolved"?
To be honest, I'm not sure whether such information in a stack is useful in
case of any exception that we may face in the RaftGroupService, however it's
definitely beyond the task description.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -811,9 +810,20 @@ private CompletableFuture<ClusterNode> resolvePeer(Peer
peer) {
ClusterNode node =
cluster.topologyService().getByConsistentId(peer.consistentId());
if (node == null) {
- return CompletableFuture.failedFuture(new ConnectException("Peer "
+ peer.consistentId() + " is unavailable"));
+ return CompletableFuture.failedFuture(new
PeerUnavailableException("Peer " + peer.consistentId() + " is unavailable"));
Review Comment:
- Please fix the recoverable() javadoc. Currently it states
`Checks if an error is recoverable, for example, {@link
java.net.ConnectException}.` but ConnectException itself isn't considered as
recoverable. BWT is a bit funny that ConnectionException wasn't handled.
- There are at least two placed where we still use ConnectException:
1. RaftGroupServiceTest
```
if (peer != null && target.name().equals(peer.consistentId())) {
return failedFuture(new ConnectException());
}
```
2. org.apache.ignite.raft.jraft.rpc.impl.IgniteRpcClient#send
```
if (targetNode == null) {
// ConnectException will force a retry by the enclosing
components.
fut.completeExceptionally(new ConnectException());
return;
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
