-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exception in GrpcRetryer. #2021
Conversation
temporal-serviceclient/src/main/java/io/temporal/internal/retryer/GrpcAsyncRetryer.java
Outdated
Show resolved
Hide resolved
temporal-serviceclient/src/main/java/io/temporal/internal/retryer/GrpcRetryerUtils.java
Outdated
Show resolved
Hide resolved
temporal-serviceclient/src/main/java/io/temporal/serviceclient/ChannelManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After internal discussion, it seems everyone agrees that no when-to-make-first-call behavior should change in the Java SDK (that would be an incompatible change), but of course fixing retry behavior and other similar ones makes sense. Will defer to @Quinn-With-Two-Ns on final approval.
7d454e3
to
5d2e9f4
Compare
temporal-serviceclient/src/main/java/io/temporal/internal/retryer/GrpcRetryer.java
Outdated
Show resolved
Hide resolved
temporal-serviceclient/src/main/java/io/temporal/serviceclient/ChannelManager.java
Outdated
Show resolved
Hide resolved
temporal-serviceclient/src/main/java/io/temporal/serviceclient/SystemInfoInterceptor.java
Show resolved
Hide resolved
5d2e9f4
to
7fcfa0f
Compare
I don't think the CI failure is a result of any of your changes, I opened a separate issue to address this flake #2038 |
As part of the PR can we please also add some unit tests to verify the changes address the issue? I understand the integration tests will live, so maybe just a targeted test of |
What was changed
ChannelManager.getServerCapabilities
follows the standard gRPC retry policy, even though it is called byGrpcRetryer
.Why?
In the course of writing new gRPC retry tests for the features repo, it was discovered that any gRPC failures returned by the server in response to
WorkflowService.getSystemInfo
will cause an uncaughtio.grpc.StatusRuntimeException
that happens before the actual gRPC retry code gets the chance to run.Checklist
Closes internal JIRA issue SDK-1888. (Sorry, I should have opened it as a GitHub issue and let it sync to JIRA instead, I didn't learn this until after the fact.)
How was this tested: using the new features repo tests in Add features tests for gRPC retry behavior features#435 .