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

Slow down initial retry attempt #333

Closed
pebo opened this issue Sep 7, 2020 · 1 comment · Fixed by #336
Closed

Slow down initial retry attempt #333

pebo opened this issue Sep 7, 2020 · 1 comment · Fixed by #336
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@pebo
Copy link

pebo commented Sep 7, 2020

Currently gaxios attempts the first retry without delay:

const delay = ((Math.pow(2, config.currentRetryAttempt) - 1) / 2) * 1000;

As config.currentRetryAttempt is 0 after the first failure the first delay will be 0ms.

If often makes sense to wait a little while before making a retry e.g. if the services responds with 429 or 503. It would be nice if retry.ts was less aggressive by default.

A workaround now is to set retryConfig.currentRetryAttempt to a non-zero value, e.g 1 before making the call to gaxios.request() but this relies on the internal implementation of the backoff logic and you'd also need to adjust retryConfig.retry..

Could the default retry implement instead be adjusted with a minimum delay value or make the backoff calculation configurable with a function that could be overridden in retryConfig e.g. something like:

function backoffDelay(currentRetryAttempt: number) {
  return ((Math.pow(2,currentRetryAttempt) - 1) / 2) * 1000;
}
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Sep 8, 2020
@bcoe bcoe added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Sep 11, 2020
@bcoe
Copy link
Contributor

bcoe commented Sep 11, 2020

I agree that we should probably stagger the first retry attempt by a few ms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants