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: make INTERNAL RST_STREAM errors retryable #947

Closed

Conversation

igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Oct 6, 2021

Normally RST_STREAM frames will get wrapped in UNAVAILABLE statuses.
However if the stream is reset higher upstream, it can trigger an INTERNAL error.
ReadRows doesn't really care what was the trigger, so this PR will treat INTERNAL
RST_STREAMs as UNAVAILABLE and retry them

Prior art:
googleapis/java-bigtable#586
googleapis/java-bigtable#1029
googleapis/java-bigquerystorage#419

Normally RST_STREAM frames will get wrapped in UNAVAILABLE statuses.
However if the stream is reset higher upstream, it can trigger an INTERNAL error.
ReadRows doesn't really care what was the trigger, so this PR will treat INTERNAL
RST_STREAMs as UNAVAILABLE and retry them
@igorbernstein2 igorbernstein2 requested review from a team as code owners October 6, 2021 14:33
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Oct 6, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 6, 2021
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like @alexander-fenster and @murgatroid99 to have a look here to make sure there isn't something more broadly applicable to be done

@murgatroid99
Copy link

Normally RST_STREAM frames will get wrapped in UNAVAILABLE statuses.

This is simply not true. RST_STREAM frames have an error code, which is analogous to HTTP/1.1 error codes, and gRPC maps RST_STREAM to a gRPC status based on the error code, not on where upstream the RST_STREAM frame originated. Most of those error codes actually map to INTERNAL, and the only one that maps to UNAVAILABLE is REFUSED_STREAM. The full mapping can be found here.

@crwilcox
Copy link
Contributor

crwilcox commented Oct 7, 2021

@murgatroid99 are there any of the codes from rst stream shown at https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#errors that are not retry safe? I think if the underlying call is idempotent they are all alright to retry?

I think particularly we are focused on error surfaced as INTERNAL, but with an inner message specifying rst_stream

@igorbernstein2 igorbernstein2 deleted the retry-rst-stream branch April 1, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants