-
Notifications
You must be signed in to change notification settings - Fork 271
feat: add ExponentialJitterBackoff
backoff strategy
#256
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -638,6 +638,36 @@ func LinearJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Resp | |
return time.Duration(jitterMin * int64(attemptNum)) | ||
} | ||
|
||
// ExponentialJitterBackoff is an extension of DefaultBackoff that applies | ||
// a jitter to avoid thundering herd. | ||
func ExponentialJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration { | ||
baseBackoff := DefaultBackoff(min, max, attemptNum, resp) | ||
|
||
if resp != nil { | ||
if retryAfterHeaders := resp.Header["Retry-After"]; len(retryAfterHeaders) > 0 && retryAfterHeaders[0] != "" { | ||
return baseBackoff | ||
} | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: To maintain consistency, can we use the same source like here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||
Comment on lines
+655
to
+658
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why is the jitter between -0.25 and +0.25 and not between Also, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As far as I can see, the
Said that, to guarantee the exponential trend of the backoff handled by Regarding your other observation, instead:
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 | ||
} | ||
|
||
Comment on lines
+661
to
+670
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This function could've been inlined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// PassthroughErrorHandler is an ErrorHandler that directly passes through the | ||
// values from the net/http library for the final request. The body is not | ||
// closed. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Default backoff already handles
Retry-After
. Is this check required again? refThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is needed to avoid adding the jitter when the
Retry-After
header has been handled, that is returning the backoff returned byDefaultBackoff
as it is.