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(bigquery): widen retry predicate #6976

Merged
merged 6 commits into from
Nov 3, 2022

Conversation

shollyman
Copy link
Contributor

This PR adds 500,504 http response codes for the default retry predicate on unary retries.

This doesn't introduce job-level retries (jobs must be recreated wholly, they can't be effectively restarted), so the primary risk of this change is terminal job state propagating into the HTTP response code of one of the polling methods (e.g. job.getQueryResults, jobs.get, etc). In this situation, job execution may appear to hang indefinitely or until context cancellation/expiry.

Related: #5248

This PR adds 500,504 http response codes for the default retry predicate
on unary retries.

This doesn't introduce job-level retries (jobs must be recreated wholly,
they can't be effectively restarted), so the primary risk of this change
is terminal job state propagating into the HTTP response code of one of
the polling methods (e.g. job.getQueryResults, jobs.get, etc).  In this
situation, the primary risk is that job execution may appear to hang
indefinitely.

Related: googleapis#5248
@shollyman shollyman requested review from alvarowolfx and a team November 1, 2022 21:39
@shollyman shollyman requested a review from a team as a code owner November 1, 2022 21:39
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the BigQuery API. labels Nov 1, 2022
@@ -237,8 +243,10 @@ func retryableError(err error, allowedReasons []string) bool {
}
}
}
if e.Code == http.StatusServiceUnavailable || e.Code == http.StatusBadGateway {
return true
for _, code := range retry5xxCodes {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function comment needs to be updated, as it mentions just the 502 and 503 errors - It also considers 502 ("Bad Gateway") and 503 ("Service Unavailable") errors retryable;

Copy link
Contributor

@alvarowolfx alvarowolfx left a comment

Choose a reason for hiding this comment

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

LGTM. But I think we might need to start some design doc or discussion around making the retry logic more configurable. Do we know of other client libraries that allow that to take some inspiration ?

@shollyman
Copy link
Contributor Author

LGTM. But I think we might need to start some design doc or discussion around making the retry logic more configurable. Do we know of other client libraries that allow that to take some inspiration ?

There's some hooks we can consider such as https://pkg.go.dev/github.com/googleapis/gax-go/v2#WithRetry

Storage rolled their own client-level settings: https://pkg.go.dev/cloud.google.com/go/storage#Client.SetRetry

@shollyman shollyman added the automerge Merge the pull request once unit tests and other checks pass. label Nov 3, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit 753b751 into googleapis:main Nov 3, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants