Add retries to all resources and datasources #286
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
Adds retries for Create and Read on the Work Pool resource.
mitchnielsen
requested a deployment
to
Acceptance Tests
GitHub Actions
Waiting
|
It probably makes sense just to implement retries on all resources/datasources at once, rather than incrementally adding them where we get reports of failures - I'm going to see if I can get Cursor to do most of the heavy lifting there, and if we can add a facility to make it easier to add/tweak in the future 🤔 |
|
@mitchnielsen trying to think, are there calls that we WOULDNT want to retry on? |
Asking in Slack 👌🏼 |
Uses the hashicorp/retryablehttp package to get an HTTP client with retries and exponential backoff out of the box. This is implemented within our client package, meaning calls to the CRUD methods from both resources and datasources will inherit that functionality.
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
mitchnielsen
changed the title
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
mitchnielsen
requested a deployment
to
Acceptance Tests
GitHub Actions
Waiting
…t into add-more-retries
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
Blocks, and other resources, need more time to resolve objects so let's bump up the defaults to accommodate that.
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
Still saw failures in acceptance tests in CI
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
Was still seeing errors in the acceptance tests
mitchnielsen
had a problem deploying
to
Acceptance Tests
GitHub Actions
Failure
- Adds a function to define how to determine whether or not to retry, extending the default function so we can accept 409s and retry 404s. We can adapt this function over time as well. - Removes the time and retry overrides, as those were not the reason that block tests were failing - the requests themselves were passing, and didn't need retries - what they needed was to retry 404s and not retry 409s.
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
Uses the stdlib http.Client interface so we don't need to specify the retryablehttp variant in every client method.
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
mitchnielsen
commented
Oct 23, 2024
| client := &Client{ | ||
| hc: http.DefaultClient, | ||
| hc: httpClient, |
There was a problem hiding this comment.
This is the main change here - we're replacing the default HTTP client with our retry-able HTTP client.
Comment on lines
-39
to
-58
| // MustNew returns a new client or panics if an error occurred. | ||
| func MustNew(opts ...Option) *Client { | ||
| client, err := New(opts...) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("error occurred during construction: %s", err)) | ||
| } | ||
|
|
||
| return client | ||
| } | ||
|
|
||
| // WithClient configures the underlying http.Client used to send | ||
| // requests. | ||
| func WithClient(httpClient *http.Client) Option { | ||
| return func(client *Client) error { | ||
| client.hc = httpClient | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These were unused.
Comment on lines
+105
to
+122
|
|
||
| func checkRetryPolicy(ctx context.Context, resp *http.Response, err error) (bool, error) { | ||
| // If the response is a 409 (StatusConflict), that means the request | ||
| // eventually succeeded and we don't need to make the request again. | ||
| if resp.StatusCode == http.StatusConflict { | ||
| return false, nil | ||
| } | ||
|
|
||
| // If the response is a 404 (NotFound), try again. This is particularly | ||
| // relevant for block-related objects that are created asynchronously. | ||
| if resp.StatusCode == http.StatusNotFound { | ||
| return true, err | ||
| } | ||
|
|
||
| // Fall back to the default retry policy for any other status codes. | ||
| //nolint:wrapcheck // we've extended this method, no need to wrap error | ||
| return retryablehttp.DefaultRetryPolicy(ctx, resp, err) | ||
| } |
There was a problem hiding this comment.
Here we can be specific about what warrants a retry. This is cool because we knew in the past we didn't need to retry 409s, but the retry implementation we had at the time didn't really let us achieve that.
mitchnielsen
commented
Oct 23, 2024
| // Finally, convert the retryablehttp client to a standard http client. | ||
| // This allows us to retain the `http.Client` interface, and avoid specifying | ||
| // the `retryablehttp.Client` interface in our client methods. | ||
| httpClient := retryableClient.StandardClient() |
There was a problem hiding this comment.
This part is pretty sweet. It saves a lot of lines changed - see f86254b.
1 task
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.
Uses the hashicorp/retryablehttp package to get an HTTP client with
retries and exponential backoff out of the box. This is implemented
within our client package, meaning calls to the CRUD methods from both
resources and datasources will inherit that functionality.
At a high level, the changes here are:
httpclient with aretryablehttpclientavast/retryin resourcesavast/retry-related helpersRelated to https://linear.app/prefect/issue/PLA-183/add-retries-to-resources