Add exponential backoff to request retries #101
Conversation
Very hard to mock a retry that uses setTimeout with random jitter
JoshMock
requested review from
pquentin,
ezimuel,
picandocodigo,
pgayvallet and
miguelgrinberg
|
Pinging all my dynamic language friends for a review, mostly because it's interesting. 🙂 |
|
|
||
| // exponential backoff on retries, with jitter | ||
| const backoff = options.retryBackoff ?? this[kRetryBackoff] | ||
| const backoffWait = backoff(0, 4, meta.attempts) |
There was a problem hiding this comment.
It seems like your intention is not to expose this as a feature to developers, so maybe this isn't important right now, but would it make sense to future-proof this and make the min and max arguments also configurable, like the attempts are?
There was a problem hiding this comment.
Good question. Nobody in the community asked for retry backoff, much less the ability to provide their own algorithm, so I think we can wait until someone actually asks for it?
src/Transport.ts
Outdated
| */ | ||
| function retryBackoff (min: number, max: number, attempt: number): number { | ||
| const ceiling = Math.min(max, 2 ** attempt) / 2 | ||
| return ceiling + ((Math.random() * (ceiling - min)) - min) |
There was a problem hiding this comment.
The expression ((Math.random() * (ceiling - min)) - min) generates a random number between -min and ceiling - 2 * min. Was this what you intended here?
There was a problem hiding this comment.
Nice catch. 😆 Fixed in 003353e.
Accidentally let the minimum be `min * -1` instead of `min`. Whoops!
|
Merging mostly so I can get this in before publishing the 8.14 client. We can continue to iterate on the implementation based on how this works, especially once Elastic Cloud SREs start to see a difference in retry handling from Kibana. |
Uses an exponential backoff algorithm that includes some jitter so that retries are not going to hammer several sequential requests at an Elasticsearch instance that might be failing or trying to catch up. Some random jitter is added to the backoff to ensure a more smooth distribution of retries.
A
retryBackoffoption is exposed on both the transport constructor andrequestfunction, but I'm not planning to document it. Exposing it is primarily to make unit testing easier. (Who would have guessed that testing code that uses time and random numbers would be frustrating?) That said, more advanced users might see value in providing their own backoff algorithm to meet their particular needs.