Skip to content

fix: retry rst stream #3000

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

Merged
merged 4 commits into from
Jun 1, 2021
Merged

fix: retry rst stream #3000

merged 4 commits into from
Jun 1, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented May 26, 2021

This was fixed in veneer client in https://github.com/googleapis/java-bigtable/pull/586/files but not fixed in bigtable-client-core.

I also noticed different error messages:

  • "Received Rst stream" mentioned in the original issue

  • "RST_STREAM closed stream" from b/189326784

  • "Received RST_STREAM" mentioned in b/179093825

So checking all 3 cases.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #2177 ☕️

@mutianf mutianf requested a review from a team as a code owner May 26, 2021 22:27
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/java-bigtable-hbase API. label May 26, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2021
@mutianf mutianf requested a review from igorbernstein2 May 26, 2021 22:49
@igorbernstein2
Copy link
Collaborator

Can you rebase on to master? The integration tests should be less flaky

@@ -263,6 +265,19 @@ protected boolean isRequestRetryable() {
return rpc.isRetryable(getRetryRequest());
}

private boolean isRstStream(Status status) {
if (!(this instanceof RetryingReadRowsOperation) || status.getCode() != Code.INTERNAL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can improve the isntanceof here

Maybe create a new method isRetryable(Status) which is:

protected boolean isRetryable(Status s)  {
  return retryOptions.isRetryable(code);
}

and override it in RetryReadRowsoperation:
@OverRide
protected boolean isRetryable(Status s) {
return isRstStream(Status status) || super.isRetryable(code);
}

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm

@mutianf mutianf merged commit 914d65f into googleapis:bigtable-1.x Jun 1, 2021
@mutianf mutianf deleted the rst_stream branch June 1, 2021 19:23
kolea2 pushed a commit to kolea2/cloud-bigtable-client that referenced this pull request Sep 10, 2021
* fix: retry rst stream

* check if operation is readrows

* Refactor

* refactor
gcf-merge-on-green bot pushed a commit that referenced this pull request Sep 10, 2021
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/java-bigtable-hbase API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants