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(storage): add retry config to BucketHandle #5170

Merged
merged 10 commits into from Dec 2, 2021

Conversation

BrennaEpp
Copy link
Contributor

@BrennaEpp BrennaEpp commented Nov 26, 2021

  • Adds Retryer configurability to BucketHandle
  • Adds retrying to BucketHandle.Update, including defaulting to retry only when idempotency conditions are present
  • Adds retry config to all direct methods on BucketHandle
  • Adds integration test for retry configs

Bucket config will merge with object config, with the object's config overriding the options it sets.

@BrennaEpp BrennaEpp requested review from a team as code owners November 26, 2021 05:15
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 26, 2021
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 26, 2021
storage/bucket.go Show resolved Hide resolved
storage/bucket.go Show resolved Hide resolved
storage/storage.go Outdated Show resolved Hide resolved
@@ -4448,6 +4449,147 @@ func findTestCredentials(ctx context.Context, envVar string, scopes ...string) (
return transport.Creds(ctx, opts...)
}

func TestIntegration_RetryConfig(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this test is a bit hacky since it depends on an unrealistic shouldRetry method. Do we really need an integration test? I feel like it's probably sufficient to just to write a unit test that verifies that the configs are what we expect them to be, if options are set on either or both of the BucketHandle and ObjectHandle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. switched it to unit test

storage/storage.go Outdated Show resolved Hide resolved
@BrennaEpp BrennaEpp enabled auto-merge (squash) December 2, 2021 19:56
@BrennaEpp BrennaEpp merged commit b2b5476 into googleapis:main Dec 2, 2021
@BrennaEpp BrennaEpp deleted the bucket-retry branch February 8, 2023 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage 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

2 participants