Extend retryablehttp limits in SDK base client #945
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
Explicitly set three fields in the `retryablehttp.Client` struct. `RetryWaitMin` is the shortest wait time before attempting to retry the request, and is the same as the default, but set explicitly here for clarity. `RetryWaitMax` is the longest wait time before attempting to retry the request. The default is 30 seconds, we extend this to 61 second in consideration of potential API throttling for especially long-lasting eventual consistency issues. `RetryMax` is the maximum number of retries for a given API request, and is given the most consideration here. The default is 4, which is far too little for our purposes. Here, we set it to 16, which in conjunction with the above two field values, and when implemented using an exponential backoff algorithm, should take approximately 10 minutes to exhaust. However, in consideration of _really_ long lasting retries, we inspect the context and increase this if it looks like the retries will be exhausted before the context deadlines. The goal is to try and deadline the context just before the retries are exhausted. For compatibility, since we don't strictly require a deadline to execute a request, we don't require one here.
manicminer
added
enhancement
tombuildsstuff
approved these changes
Mar 27, 2024
| @@ -672,6 +672,18 @@ func (c *Client) retryableClient(checkRetry retryablehttp.CheckRetry) (r *retrya | |||
| r.CheckRetry = checkRetry | |||
| r.ErrorHandler = RetryableErrorHandler | |||
| r.Logger = log.Default() | |||
| r.RetryWaitMin = 1 * time.Second | |||
| r.RetryWaitMax = 61 * time.Second | |||
There was a problem hiding this comment.
FWIW some ARM API's do have Retry-After headers with an excessive timeout (e.g. 10m) - but I don't see any harm in us polling more regularly given that extended interval so 👍
There was a problem hiding this comment.
Oh if the response includes a Retry-After header, that is always respected/selected first before falling back to exponential backoff (i.e. these are not used in that case)
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.
Explicitly set three fields in the
retryablehttp.Clientstruct.RetryWaitMinis the shortest wait time before attempting to retry the request, and is the same as the default, but set explicitly here for clarity.RetryWaitMaxis the longest wait time before attempting to retry the request. The default is 30 seconds, we extend this to 61 second in consideration of potential API throttling for especially long-lasting eventual consistency issues.With these values, the retry curve looks something like this.
RetryMaxis the maximum number of retries for a given API request, and is given the most consideration here. The default is 4, which is far too little for our purposes. Here, we set it to 16, which in conjunction with the above two field values, and when implemented using an exponential backoff algorithm, should take approximately 10 minutes to exhaust.However, in consideration of really long lasting retries, we inspect the context and increase this if it looks like the retries will be exhausted before the context deadlines. The goal is to try and deadline the context just before the retries are exhausted.
For compatibility, since we don't strictly require a deadline to execute a request, we don't require one here.
Related: hashicorp/terraform-provider-azurerm#25397