Skip to content
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

GrpcRetryer could lose a real cause in case of DEADLINE_EXCEEDED #666

Closed
Spikhalskiy opened this issue Aug 24, 2021 · 0 comments · Fixed by #674
Closed

GrpcRetryer could lose a real cause in case of DEADLINE_EXCEEDED #666

Spikhalskiy opened this issue Aug 24, 2021 · 0 comments · Fixed by #674
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Aug 24, 2021

If GrpcRetryer gets some retryable exception enough times that it ends in DEADLINE_EXCEEDED, it will throw DEADLINE_EXCEEDED as the main exception outside, losing information about previous exceptions that actually made us continue retrying and this can hurt debuggability and ease of investigations for our users. Because most application developers will be actually interested in the underlying exception that made us retrying all this time.

Proposed solution:
If DEADLINE_EXCEEDED happened during retries, we can discard DEADLINE_EXCEEDED and return a previous exception if it exists.
It's probably the cleanest way because it simulates what happens during a normal timeout.
But what if the code that uses GrpcRetryer relies on DEADLINE_EXCEEDED exception to detect the expiration of GRPC deadline? It will quickly get DEADLINE_EXCEEDED on the next GRPC operation. We need to discuss if it's good enough and if this approach is safe.

@Spikhalskiy Spikhalskiy added bug Something isn't working TBD This needs a broader discussion before implementing labels Aug 24, 2021
@Sushisource Sushisource changed the title GrpcRetryer could loose a real cause in case of DEADLINE_EXCEEDED GrpcRetryer could lose a real cause in case of DEADLINE_EXCEEDED Aug 24, 2021
@Spikhalskiy Spikhalskiy added this to the v1.3.0 milestone Aug 25, 2021
@Spikhalskiy Spikhalskiy removed the TBD This needs a broader discussion before implementing label Aug 25, 2021
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Aug 26, 2021
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Aug 26, 2021
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Aug 26, 2021
vkoby pushed a commit to vkoby/sdk-java that referenced this issue Aug 30, 2021
@Spikhalskiy Spikhalskiy self-assigned this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant