Skip to content

fix: retry rst stream#3000

Merged
mutianf merged 4 commits into
googleapis:bigtable-1.xfrom
mutianf:rst_stream
Jun 1, 2021
Merged

fix: retry rst stream#3000
mutianf merged 4 commits into
googleapis:bigtable-1.xfrom
mutianf:rst_stream

Conversation

@mutianf

@mutianf mutianf commented May 26, 2021

Copy link
Copy Markdown
Contributor

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 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
Copy Markdown
Collaborator

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

}

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

Copy link
Copy Markdown
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);
}

@igorbernstein2 igorbernstein2 left a comment

Copy link
Copy Markdown
Collaborator

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