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

Clean up retryPolicies #734

Closed
3 tasks done
aoberoi opened this issue Mar 12, 2019 · 0 comments
Closed
3 tasks done

Clean up retryPolicies #734

aoberoi opened this issue Mar 12, 2019 · 0 comments
Labels
enhancement M-T: A feature request for new functionality semver:major

Comments

@aoberoi
Copy link
Contributor

aoberoi commented Mar 12, 2019

Description

The current names for retry policies exported from the package are not very accurate at describing what they do. Let's make it a little easier.

Old Description
retryForeverExponential retries with the default timeout settings, in a repeating pattern, forever
retryForeverExponentialCapped same as above, except the maximum length of a timeout is 30 minutes, but the pattern will still repeat forever
retryForeverExponentialCappedRandom (default) same as above, but there's also some randomness introduced to prevent stampeding herds
fiveRetriesInFiveMinutes what it says
rapidRetryPolicy try 10 times in about 10ms

Specifically, retryForeverExponentialCapped doesn't make sense because the maximum timeout that can be reached is about 17 minutes, so the 30 minute cap is meaningless.

Let's simplify these to the following:

New Description
tenRetriesInAboutThirtyMinutes (default) what it says
fiveRetriesInFiveMinutes what it says
rapidRetryPolicy try 10 times in about 10ms

Notable, the "about" word is helpful for showing that the randomize option is being used. Also notable, the forever option is not being used, so that errors that retry don't stay "stuck" in a retry loop. This may cause more errors to be seen, but that is likely a good thing so that developers can find issues in their apps.

The tenRetriesInAboutThirtyMinutes policy looks like this:

{
  retries: 10,
  factor: 1.96821,
  randomize: true,
}

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added semver:major enhancement M-T: A feature request for new functionality labels Mar 12, 2019
@aoberoi aoberoi added this to the v5 - Major version update milestone Mar 12, 2019
aoberoi added a commit to aoberoi/node-slack-sdk that referenced this issue Mar 12, 2019
@aoberoi aoberoi mentioned this issue Mar 12, 2019
2 tasks
@aoberoi aoberoi closed this as completed Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:major
Projects
None yet
Development

No branches or pull requests

1 participant