feat(internal/gensupport): add configurable retry #1324
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extend SendAndRetryRequest to allow retry options (backoff and ErrorFunc) to be passed in. Add a surface to the generated code for storage to allow the manual layer to choose whether the request is retried. I tested this out via the manual layer by verifying that the ObjectsInsertCall only retries now when WithRetry() is applied. I still need to update the generator to ensure that the changes in the generated file will persist and probably add some tests as well. Thought I'd try and get an initial review first.
google-cla
bot
added
the
cla: yes
tritone
added a commit
to tritone/google-cloud-go
that referenced
this pull request
Dec 9, 2021
Depends on merge and release of googleapis/google-api-go-client#1324
frankyn
reviewed
Dec 10, 2021
| "which errors are considered retryable. By default, exponetial backoff will be" + | ||
| "applied using gax defaults, and the following errors are retried:" + | ||
| "\n\n" + | ||
| "- HTTP responses with codes 429, 502, 503, and 504." + |
There was a problem hiding this comment.
Could the error codes be pulled from gax instead of string literals?
There was a problem hiding this comment.
Seeing as this is just a one-off comment and these values are not likely to change, I don't think it's work adding extra complexity to the templating in order to accomplish this.
| @@ -50,7 +52,7 @@ func send(ctx context.Context, client *http.Client, req *http.Request) (*http.Re | |||
| // If ctx is non-nil, it calls all hooks, then sends the request with | |||
| // req.WithContext, then calls any functions returned by the hooks in | |||
| // reverse order. | |||
| func SendRequestWithRetry(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) { | |||
| func SendRequestWithRetry(ctx context.Context, client *http.Client, req *http.Request, retry *RetryConfig) (*http.Response, error) { | |||
There was a problem hiding this comment.
The only thing I think will be an issue is that the remote offset is not considered in the case Storage has progressed and the client is now unaligned.
There was a problem hiding this comment.
Yeah, that work will have to come in a separate PR. This PR doesn't change the mechanics of the resumption strategy.
frankyn
approved these changes
Dec 13, 2021
tritone
added a commit
to tritone/google-cloud-go
that referenced
this pull request
Dec 14, 2021
Depends on merge and release of googleapis/google-api-go-client#1324
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend SendAndRetryRequest to allow retry options (backoff and
ErrorFunc) to be passed in. Add a surface to the generated code
for storage to allow the manual layer to choose whether the request
is retried.
Idempotency considerations will be handled at the manual layer.
I tested this out via the manual layer by verifying that the
ObjectsInsertCall only retries now when WithRetry() is applied.
I still need to update the generator to ensure that the changes in
the generated file will persist and probably add some tests as well.
Thought I'd try and get an initial review first.