Add a new RateLimitLinearJitterBackoff policy #260
Conversation
miagilepner
changed the title
jackofallops
left a comment
jackofallops
left a comment
There was a problem hiding this comment.
THanks @miagilepner - LGTM 👍
abhijeetviswa
left a comment
abhijeetviswa
left a comment
There was a problem hiding this comment.
The changes look fine. However, does it makes sense to
| if resp != nil { | ||
| if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode == http.StatusServiceUnavailable { | ||
| if sleep, ok := parseRetryAfterHeader(resp.Header["Retry-After"]); ok { | ||
| return sleep | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Does it makes sene to ignore Retry-After if it isn't 503 or 429? From what I've understood, the spec doesn't specify the status codes for which this header is valid but instead identifies what the header means for specific status codes.
There was a problem hiding this comment.
I believe the only other case where a Retry-After header should be sent is with a redirect status. The HTTP client will do redirecting as part of c.HTTPClient.Do, before we get to any retrying, so it's unlikely that we'll end up here with a 3xx status code, but I suppose it's possible.
I can update this to parse the header regardless of status code
There was a problem hiding this comment.
Tbh, I'm not really sure what is the correct way to handle the header. If there is a req to handle other status codes, we'll deal with it then. I'll approve the PR for now. Thanks!
There was a problem hiding this comment.
I see you've already push a commit to ignore the status code. I'm okay with either way of handling the header.
| // RateLimitLinearJitterBackoff wraps the retryablehttp.LinearJitterBackoff. | ||
| // It first checks if the response status code is http.StatusTooManyRequests | ||
| // (HTTP Code 429) or http.StatusServiceUnavailable (HTTP Code 503). If it is | ||
| // and the response contains a Retry-After response header, it will wait the | ||
| // amount of time specified by the header. Otherwise, this calls | ||
| // LinearJitterBackoff. |
There was a problem hiding this comment.
After removing the status codes checks, the documentation is no longer correct. Let's either fix this or undo the commit.
There was a problem hiding this comment.
Reverted the commit
miagilepner
force-pushed
the
miagilepner/VAULT-36112-retry-after-header
branch
from
a958a6a to
7096c34
Compare
3 participants
This backoff combines checking retry headers with linear jitter. This is used by both Boundary and Vault, so it makes sense to have it in the shared library.