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

[HttpClient] Add jitter to RetryBackoff #38434

Merged
merged 1 commit into from Oct 7, 2020
Merged

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Oct 6, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR TODO

From the idea https://twitter.com/mtdowling/status/1313205613158043648 this PR adds a new jitter parameter to the ExponentialBackOff implementation.

jitter is a percentage (float between 0 and 1) of randomness to apply to the computed delay.
ie. if the initial delay is 1000ms, and jitter=0.2, the finale delay will be an number between 800 and 1200 (1000 +/- 20%)

@javiereguiluz
Copy link
Member

javiereguiluz commented Oct 6, 2020

If I understand this feature, when using 1000ms and a multiplier of 3, instead of waiting this:

1000ms   3000ms   9000ms

It would wait something like this:

800ms    3150ms   8830ms

I can't understand why the second is best for the server or for the local application. Thanks.

@jderusse
Copy link
Member Author

jderusse commented Oct 6, 2020

I can't understand why the second is best for the server or for the local application. Thanks.

When several clients perform the same request at the same time (ie. started by a crontab) they all call the server at the exact same time, and have a highest probability to trigger an error then they all retries at the exact same time.

More documentation here https://aws.amazon.com/builders-library/timeouts-retries-and-backoff-with-jitter/?did=ba_card&trk=ba_card and here https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/

@stof
Copy link
Member

stof commented Oct 6, 2020

@javiereguiluz if you are the only client, the jitter has indeed no value. The value appears when multiple clients apply such jitter:

Without jitter:

A: 1000ms   3000ms   9000ms
B: 1000ms   3000ms   9000ms

With jitter:

A:  800ms    3150ms   8830ms
B: 1100ms    2953ms   8900ms

@javiereguiluz
Copy link
Member

Thanks for the comments ... but isn't a bit unlikely that your app starts more than one HTTP client making requests to the same server at the exact same time? I'm probably missing something, because I like this idea in theory but I don't say any significant difference in practice. But I guess some folks using Symfony will fit this scenario so they'll like the new feature. Thanks!

@nicolas-grekas
Copy link
Member

@javiereguiluz This can happen trivially when making concurrent requests to a rate-limited endpoint (eg the github API).

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about retry-after headers?
also, do we care about the order at all?

@jderusse
Copy link
Member Author

jderusse commented Oct 6, 2020

what about retry-after headers?

yeah, I wondered if it should bellongs to ExponentialBackOff or to the RetryableHttpClient in order to add jitter everywhere.

On the other hand, the server provide the retry-after header. It should be able to distribute load on all clients.

@stof
Copy link
Member

stof commented Oct 6, 2020

but isn't a bit unlikely that your app starts more than one HTTP client making requests to the same server at the exact same time?

there is not even a need for both clients to be part of the same app.

@stof
Copy link
Member

stof commented Oct 6, 2020

and indeed, A and B don't need to be 2 different clients in your app. they might be 2 concurrent requests done by the same client

@jderusse jderusse changed the base branch from master to 5.x October 6, 2020 21:46
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

Thank you @jderusse.

@fabpot fabpot merged commit 494ef42 into symfony:5.x Oct 7, 2020
@fabpot fabpot mentioned this pull request Oct 14, 2020
@jderusse jderusse deleted the retry-jitter branch October 15, 2020 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants