Skip to content
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

Support random jitter combined with other delay options. #26

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Support random jitter combined with other delay options. #26

merged 1 commit into from
Jan 28, 2020

Conversation

acourtneybrown
Copy link
Contributor

Issue #9 initially proposed having a random jitter component to the delay; however, PR #12 omitted the random jitter. This PR adds both a RandomDelay (for jitter) and CombineDelay (to allow combining any number of delays). In addition, the default delay changes from exponential backoff (BackOffDelay) to exponential backoff with random jitter (BackOffDelay & RandomDelay).

Adds `RandomDelay` and `CombineDelay` as `DelayTypeFunc`s
Adds `CombineDelay` which allows multiple `DelayTypeFunc`s to b chained together.
Updates the default `DelayTypeFunc` to exponential backoff with random jitter.
@JaSei
Copy link
Collaborator

JaSei commented Jan 25, 2020

Hi @acourtneybrown, thanks for your PR. I like the idea of ​​RandomDelay and CombineDelay. What I don't sure, is change the default from BackOffDelay to combine of BackOffDelay and RandomDelay. Why do you prefer this default? thanks

@acourtneybrown
Copy link
Contributor Author

Hey @JaSei, I won't be able to state it nearly as completely as StackOverflow or the AWS blog, but basically, adding the random jitter to the delay helps avoid a large number of similar clients from retrying at the same time after earlier failures. I could easily see reducing the default jitter from the max of 100ms in the current PR.

Can you explain why you might prefer BackOffDelay over the combination of BackOffDelay and RandomDelay?

@JaSei
Copy link
Collaborator

JaSei commented Jan 28, 2020

Thanks to references - it make sense...

@JaSei JaSei merged commit 88a508d into avast:master Jan 28, 2020
@JaSei
Copy link
Collaborator

JaSei commented Jan 28, 2020

It was released as version 2.5.0
Thanks for your PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants