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

gensupport: only retry storage media upload requests #538

Merged
merged 5 commits into from
Jun 18, 2020

Conversation

tritone
Copy link
Contributor

@tritone tritone commented Jun 17, 2020

This modifies the changes in #528 by reverting the changes
to SendRequest (which is widely used) and instead moving the
new logic to a separate function, SendAndRetryRequest which
will only be used on the initial request for media uploads.

There is some code duplication from SendRequest now, which I
can de-duplicate if folks think this is a good approach

Fixes googleapis/google-cloud-go#2469

This modifies the changes in googleapis#528 by reverting the changes
to SendRequest (which is widely used) and instead moving the
new logic to a separate function, SendAndRetryRequest which
will only be used on the initial request for media uploads.

There is some code duplication from SendRequest now, which I
can de-duplicate if folks think this is a good approach

Fixes googleapis#2469
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 17, 2020
@broady
Copy link
Contributor

broady commented Jun 17, 2020

CL title should probably say something like "only retry media upload requests"?

The method name is an internal detail (gensupport is internal). But if someone were to look through commit history, this would explain a change in behavior between versions (in addition to release notes of course)

@broady
Copy link
Contributor

broady commented Jun 17, 2020

er, PR title :)

@tritone
Copy link
Contributor Author

tritone commented Jun 17, 2020

@broady good point, thanks. :)

@tritone tritone changed the title gensupport: add SendAndRetryRequest gensupport: only retry media upload requests Jun 17, 2020
@tritone
Copy link
Contributor Author

tritone commented Jun 18, 2020

Discussed this with @codyoss offline-- we do think it makes sense to limit scope to storage for right now until the effects have been more fully evaluated. It ended up being a simple change in gen.go to accomplish this (I verified by running the generation locally and confirming that only the one call I was targeting in storage-gen.go is now impacted once generation runs).

@tritone tritone changed the title gensupport: only retry media upload requests gensupport: only retry storage media upload requests Jun 18, 2020
@tritone tritone requested a review from frankyn June 18, 2020 02:59
@tritone tritone merged commit ae8953e into googleapis:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: TestIndefiniteRetries failed
5 participants