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.

Sign up for GitHub

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

Jump to bottom

feat: handle retry info so client respect the delay server sets #2026

Merged
merged 14 commits into from Dec 19, 2023
Merged

feat: handle retry info so client respect the delay server sets #2026

merged 14 commits into from Dec 19, 2023

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Dec 6, 2023 •
edited

Creates a new retry algorithm that checks if the throwable has a RetryInfo field, and use the retry_delay in the RetryInfo as the backoff time for the next retry.

Test and rollback plan: https://docs.google.com/document/d/1Kim-80vUSDgYxDVrFtkl8NkV-SsW8KAor_Ca1rpvbyo/edit?resourcekey=0-oV2CmU7_sZEMpPMFF9ZSOA&tab=t.0#bookmark=id.mtpgcav79p4u

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)
  • Rollback plan is reviewed and LGTMed

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@mutianf mutianf requested a review from a team as a code owner December 6, 2023 19:25
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/java-bigtable API. labels Dec 6, 2023
@mutianf mutianf requested a review from a team as a code owner December 6, 2023 20:14
@mutianf mutianf force-pushed the retry_info branch 3 times, most recently from 3a6040c to 491ae3d Compare December 8, 2023 14:39
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Dec 14, 2023
igorbernstein2
igorbernstein2 requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@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.

Also please update the pr description with:

  • how this was tested e2e
  • a link to a rollback doc

...-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiResultRetryAlgorithm.java Outdated Show resolved Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiResultRetryAlgorithm.java Outdated Show resolved Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiResultRetryAlgorithm.java Outdated Show resolved Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiResultRetryAlgorithm.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 17, 2023
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Dec 18, 2023
igorbernstein2
igorbernstein2 reviewed Dec 18, 2023
View reviewed changes
...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiException.java Outdated

// TODO: move this to gax later
@InternalApi
public class ApiException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ApiExceptions. Also please add a private ctor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In gax it is ApiException I think https://github.com/googleapis/sdk-platform-java/blob/main/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiException.java

Copy link
Contributor

Choose a reason for hiding this comment

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

In general the pattern is that the instance class is singular and the helper utility with static methods is plural.
In this case you are adding latter. Eventually when you upstream, you will make it an instance method and it will be in ApiException. But in the transitional state I think plural makes sense

...-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java Outdated Show resolved Hide resolved
google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java Show resolved Hide resolved
igorbernstein2
igorbernstein2 approved these changes Dec 19, 2023
View reviewed changes
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiException.java Outdated

// TODO: move this to gax later
@InternalApi
public class ApiException {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general the pattern is that the instance class is singular and the helper utility with static methods is plural.
In this case you are adding latter. Eventually when you upstream, you will make it an instance method and it will be in ApiException. But in the transitional state I think plural makes sense

@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2023
@mutianf mutianf merged commit f1b7fc7 into googleapis:main Dec 19, 2023
20 checks passed
@mutianf mutianf deleted the retry_info branch December 19, 2023 17:14
@release-please release-please bot mentioned this pull request Dec 19, 2023
Merged
kolea2
kolea2 reviewed Dec 19, 2023
View reviewed changes
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiExceptions.java

import com.google.api.core.InternalApi;

// TODO: move this to gax later
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have a plan for when we're moving this to gax?

gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 16, 2024
@release-please
chore(main): release 2.31.0 (#2034)
cc75364 
🤖 I have created a release *beep* *boop*
---


## [2.31.0](https://togithub.com/googleapis/java-bigtable/compare/v2.30.0...v2.31.0) (2024-01-12)


### Features

* Add a flag to add / remove routing cookie from callable chain ([#2032](https://togithub.com/googleapis/java-bigtable/issues/2032)) ([201e631](https://togithub.com/googleapis/java-bigtable/commit/201e631f893b1edacdd5760c1d180b212dc9e38a))
* Adding feature flags for routing cookie and retry info ([#2031](https://togithub.com/googleapis/java-bigtable/issues/2031)) ([08c5bf1](https://togithub.com/googleapis/java-bigtable/commit/08c5bf1fd76258387135c8c3abe75f13bcdcc1f6))
* Count row merging errors as internal errors ([#2045](https://togithub.com/googleapis/java-bigtable/issues/2045)) ([fc7845b](https://togithub.com/googleapis/java-bigtable/commit/fc7845bd4cefca05bccc4dc3a9f727fd20f5adf6))
* Enable feature flag when setting is enabled ([#2043](https://togithub.com/googleapis/java-bigtable/issues/2043)) ([e0d90db](https://togithub.com/googleapis/java-bigtable/commit/e0d90db67b3ea52d833f7d6bcd78e3f7e91ff301))
* Handle retry info so client respect the delay server sets ([#2026](https://togithub.com/googleapis/java-bigtable/issues/2026)) ([f1b7fc7](https://togithub.com/googleapis/java-bigtable/commit/f1b7fc79ad3fd9006e430e48430331b360bb22e3))


### Bug Fixes

* **deps:** Update the Java code generator (gapic-generator-java) to 2.31.0 ([#2044](https://togithub.com/googleapis/java-bigtable/issues/2044)) ([d9042a5](https://togithub.com/googleapis/java-bigtable/commit/d9042a567f284424efb4af69f757883c9781dce3))
* Fix RetryInfo algorithm and tests ([#2041](https://togithub.com/googleapis/java-bigtable/issues/2041)) ([dad7517](https://togithub.com/googleapis/java-bigtable/commit/dad751736112323c578b3c90d9587fc182105747))


### Dependencies

* Update dependency com.google.cloud:gapic-libraries-bom to v1.27.0 ([#2030](https://togithub.com/googleapis/java-bigtable/issues/2030)) ([a492d02](https://togithub.com/googleapis/java-bigtable/commit/a492d02bdc52cb81d8804a4d7cd363b5807bdd47))
* Update dependency com.google.truth.extensions:truth-proto-extension to v1.2.0 ([#2035](https://togithub.com/googleapis/java-bigtable/issues/2035)) ([46e1e03](https://togithub.com/googleapis/java-bigtable/commit/46e1e0335f9969fa1b60acdf17e9b8abbc312ca2))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolea2 kolea2 kolea2 left review comments

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

Assignees
No one assigned
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mutianf @igorbernstein2 @kolea2