Unwrap CompletionException in GrpcClient #298
Conversation
|
|
|
Hi @marcuslyth, thank you for your contribution! Great catch. That said, I noticed that your patch will conflict with #295. To ensure you remain credited as the original author of the fix, I’d prefer not to resolve it directly in #295. Once #295 is merged, would you mind rebasing your work onto the trunk branch? Thanks again for your time and effort! |
You are more than welcome! Sure thing, as soon as #295 have been merged I can rebase my change. |
|
Hey @marcuslyth, #295 is finally merged. Could you rebase your branch? |
GrpcClient expects Exceptions of a specific type but as it is right now it will not handle any exceptions as they are propagated as CompletionException This will not solve any similar issues for any other class using ClientTelemetry, and a better solution would probably be to never wrap the exception, however that would require more intrusive changes to the future chain. Fixes kurrent-io#287
|
Done! |
|
After all you rebased from master, did you patch is still behaving properly? |
Yes, I have tested it both on unit level and by building the client and using it in one of our services with a three node setup and it works as expected. |
|
Thanks for your work! |
GrpcClient expects Exceptions of a specific type but as it is right now it will not handle any exceptions as they are propagated as CompletionException
This will not solve any similar issues for any other class using ClientTelemetry, and a better solution would probably be to never wrap the exception, however that would require more intrusive changes to the future chain.
Fixes #297