-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
only retry downloadtool on 500s and 408 and 429 #373
Conversation
let attempt = 1 | ||
while (attempt < this.maxAttempts) { | ||
// Try | ||
try { | ||
return await action() | ||
} catch (err) { | ||
if (isRetryable && !isRetryable(err)) { | ||
throw err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was happy to learn nodejs retains the original stack trace when an exception is rethrown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thoughts
if (err instanceof HTTPError && err.httpStatusCode) { | ||
// Don't retry anything less than 400 | ||
// Don't retry 404 Not Found | ||
if (err.httpStatusCode < 400 || err.httpStatusCode === 404) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most 4xx responses are not retryable, the only one we should really retry for is 408- Request timeout and 429- Rate Limiting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
expect(info).toHaveLength(0) | ||
}) | ||
|
||
it('checks retryable after second attempt', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i want to learn
835cbef
to
6797a48
Compare
6797a48
to
728942f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
728942f
to
abde3c6
Compare
abde3c6
to
3dedf1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
expect(info).toHaveLength(0) | ||
}) | ||
|
||
it('checks retryable after second attempt', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i want to learn
downloadtool should only retry on 500s and 408 and 429