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

feat: segregate retry decision for read and write APIs #1465

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

pratik151192
Copy link
Contributor

  • chore: add tests for retry strategies
  • feat: segregate retry decision for read and write APIs

@pratik151192 pratik151192 marked this pull request as ready for review November 12, 2024 20:00
anitarua
anitarua previously approved these changes Nov 12, 2024
Copy link
Contributor

@anitarua anitarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few questions:

  1. We currently only retry idempotent RPCs. On a CANCELLED, shouldn't we retry any idempotent RPC (vs only read APIs)? If we can retry any idempotent RPC, then we won't have to maintain separate read vs write RPC lists.

  2. I understand this came up because we want the client to be tolerant to deploys. Will the retry schedule we have now be long enough for outages due to deployments?

@pratik151192
Copy link
Contributor Author

pratik151192 commented Nov 12, 2024

@malandis

  1. Good call I thought of that too but wanted to be a bit safe but I think we can do that now you mention it as well. "Safe" and "idempotent" go hand in hand, set is also safe to retry as if it was accepted will just overwrite the same value same as unavailable and internal.

  2. Nah these are very brief; similar to internal and unavailable.

I'll revert the source file and just uncomment cancelled

@pratik151192
Copy link
Contributor Author

pratik151192 commented Nov 12, 2024

@malandis To add more context, not all requests are bound to this so the length of deployment is unrelated. It's more to do with how grpc-js is handling GOAWAY frames and in some cases (not totally known) it doesn't respect it and cancels the RPC instead. More details here grpc/grpc-node#2694

These are low enough in volume and historically has been to not be impacted by deployment length.

@pratik151192 pratik151192 merged commit 2e66ed6 into main Nov 12, 2024
13 checks passed
@pratik151192 pratik151192 deleted the segregate-read-write-retry-strategies branch November 12, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants