Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

Need to Delay Retry #164

Merged
merged 4 commits into from
Apr 13, 2018
Merged

Need to Delay Retry #164

merged 4 commits into from
Apr 13, 2018

Conversation

iwanjunaid
Copy link
Contributor

@iwanjunaid iwanjunaid commented Apr 12, 2018

Sometimes we need to delay retry to avoid high CPU consumes during retry, especially when we set retryCount to big number.

@codecov-io
Copy link

codecov-io commented Apr 12, 2018

Codecov Report

Merging #164 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #164   +/-   ##
=======================================
  Coverage   46.29%   46.29%           
=======================================
  Files           1        1           
  Lines         108      108           
=======================================
  Hits           50       50           
  Misses         58       58
Impacted Files Coverage Δ
index.js 46.29% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e2aca0...b71100c. Read the comment docs.

@f2prateek
Copy link
Contributor

I think axiosRetry.exponentialDelay might be a better option here. https://github.com/softonic/axios-retry

Exponential backoff is generally a good idea and we use that approach in some of the other lirbaries.

@f2prateek
Copy link
Contributor

We also don't need to make it an option - let's make exponential retry the default. If folks want control, we can always add it later.

@iwanjunaid
Copy link
Contributor Author

iwanjunaid commented Apr 12, 2018

Thank you for your suggestion. I think, yes we should use exponential delay.

@iwanjunaid
Copy link
Contributor Author

iwanjunaid commented Apr 13, 2018

Hi @f2prateek What do you think? Is the PR ok or need some improvements?

@f2prateek
Copy link
Contributor

LGTM! Would love to get @vadimdemedes to take a look as well

@f2prateek f2prateek self-requested a review April 13, 2018 05:20
@f2prateek f2prateek merged commit 5c3bdad into segmentio:master Apr 13, 2018
@iwanjunaid
Copy link
Contributor Author

iwanjunaid commented Apr 20, 2018

Hi guys,

Just curious when will you guys publish these changes to NPM?

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

Successfully merging this pull request may close these issues.

4 participants