Fixed GrpcRetryer#retryWithResultAsync implementation #477
Conversation
Spikhalskiy
force-pushed
the
issue-476
branch
4 times, most recently
from
0b6787e to
f5b0ade
Compare
| }); | ||
| fail("unreachable"); | ||
| } catch (Exception e) { | ||
| // I'm not sure it's a good idea to replace interrupted exception with CancellationException, |
There was a problem hiding this comment.
Please note this comment. I don't think it's a good practice to shadow InterruptedExcption like this even while we save the interrupted flag in GrpcRetryer. Also, it's inconsistent with the Async version of this method.
| return new ValueExceptionPair<>(CompletableFuture.completedFuture(r), null); | ||
| } | ||
| if (e instanceof CompletionException) { | ||
| e = e.getCause(); |
There was a problem hiding this comment.
Can you leave a comment explaining why we do what we do?
Also it's possible for CompletionException to be constructed without cause, should we still do this trick in that case?
There was a problem hiding this comment.
Can you leave a comment explaining why we do what we do?
I added the comment, also there are tests for both wrapped and unwrapped exceptions added in this PR.
Also it's possible for CompletionException to be constructed without cause, should we still do this trick in that case?
It's kinda an illegal state. CompletionException is intended to be a wrapper over another exception. But it's technically possible and it will work correctly: The next if check will be true and we will return without retrying because GrpcRetrier retries only StatusRuntimeException.
2 participants
What was changed:
GrpcRetryer#retryWithResultAsyncimplementation was fixed to correctly unwrapCompletionExceptionTests provided for
GrpcRetryer.The unused
Retryerclass is deleted.Closes issue:
Issue #476
How was this tested:
Basic unit tests for GrpcRetryer were added