feat: add ExponentialJitterBackoff backoff strategy#256
feat: add ExponentialJitterBackoff backoff strategy#256wandering-tales wants to merge 1 commit intohashicorp:mainfrom
ExponentialJitterBackoff backoff strategy#256Conversation
|
|
| func ExponentialJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration { | ||
| baseBackoff := DefaultBackoff(min, max, attemptNum, resp) | ||
|
|
||
| if resp != nil { |
There was a problem hiding this comment.
Question: Default backoff already handles Retry-After. Is this check required again? ref
There was a problem hiding this comment.
This check is needed to avoid adding the jitter when the Retry-After header has been handled, that is returning the backoff returned by DefaultBackoff as it is.
| } | ||
|
|
||
| // Seed randomization; it's OK to do it every time | ||
| rnd := rand.New(rand.NewSource(time.Now().UnixNano())) |
There was a problem hiding this comment.
Suggestion: To maintain consistency, can we use the same source like here?
There was a problem hiding this comment.
I can factor out a new function to get the randomization source. Is that what you mean?
| jitter := rnd.Float64()*0.5 - 0.25 // Random value between -0.25 e +0.25 | ||
| jitteredSleep := time.Duration(float64(baseBackoff) * (1.0 + jitter)) | ||
|
|
||
| return clampDuration(jitteredSleep, min, max) |
There was a problem hiding this comment.
Question: Why is the jitter between -0.25 and +0.25 and not between min and max like in LinearJitterBackoff?
Also, since min and max mean different things between the two functions, it would be worthwhile adding it to the function documentation.
There was a problem hiding this comment.
Question: Why is the jitter between -0.25 and +0.25 and not between min and max like in LinearJitterBackoff?
As far as I can see, the min and max arguments should be respectively treated as absolute lower and
upper limits for the computed backoff. In that regard, the LinearJitterBackoff function is the only one that treats them slightly differently. Quoting its documentation:
min and max here are not absolute values. The number to be multiplied by
the attempt number will be chosen at random from between them, thus they are
bounding the jitter.
Said that, to guarantee the exponential trend of the backoff handled by DefaultBackoff, I think the jitter should necessarily be within a "neighborhood", in a mathematical sense, of the value returned by DefaultBackoff. Hence, a random value is picked between the interval [ backoff - 0.25 * backoff, backoff + 0.25 * backoff ].
Regarding your other observation, instead:
Also, since min and max mean different things between the two functions, it would be worthwhile adding it to the function documentation.
I'll improve the documentation of the new function to explain how the arguments are used.
| func clampDuration(d, min, max time.Duration) time.Duration { | ||
| if d < min { | ||
| return min | ||
| } | ||
| if d > max { | ||
| return max | ||
| } | ||
| return d | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: This function could've been inlined.
There was a problem hiding this comment.
The reason for this function to exist is to fully test it in isolation.
As this clamping is performed for a random value, there's no guarantee of full test coverage.
|
@abhijeetviswa Sorry for the noise in the unit tests. Fixing those as well. |
The new strategy is an extension of the default one that applies a jitter to avoid thundering herd.
2 participants
The new strategy is an extension of the default one that applies a jitter to avoid thundering herd.