Skip to content

only retry downloadtool on 500s and 408 and 429 - #373

Merged
ericsciple merged 1 commit into
masterfrom
users/ericsciple/m167retry
Mar 9, 2020
Merged

ericsciple merged 1 commit into
masterfrom
users/ericsciple/m167retry

Conversation

@ericsciple

@ericsciple ericsciple commented Mar 9, 2020 •

Copy link
Copy Markdown
Contributor

downloadtool should only retry on 500s and 408 and 429

return await action()
} catch (err) {
if (isRetryable && !isRetryable(err)) {
throw err

@ericsciple ericsciple Mar 9, 2020 •

Copy link
Copy Markdown
Contributor Author

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

Comment thread packages/tool-cache/src/tool-cache.ts
@ericsciple ericsciple changed the title [draft] dont retry downloadtool on 404 dont retry downloadtool on 404 Mar 9, 2020
@ericsciple
ericsciple requested a review from thboop March 9, 2020 14:45

@thboop thboop left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thoughts

Comment thread packages/tool-cache/src/tool-cache.ts Outdated
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) {

Copy link
Copy Markdown
Collaborator

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment thread packages/tool-cache/src/tool-cache.ts
expect(info).toHaveLength(0)
})

it('checks retryable after second attempt', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to learn

@ericsciple
ericsciple force-pushed the users/ericsciple/m167retry branch from 835cbef to 6797a48 Compare March 9, 2020 16:01
@ericsciple ericsciple changed the title dont retry downloadtool on 404 dont retry downloadtool on 400s except 408 and 429 Mar 9, 2020
@ericsciple ericsciple changed the title dont retry downloadtool on 400s except 408 and 429 only retry downloadtool on 500s and 408 and 429 Mar 9, 2020
@ericsciple
ericsciple force-pushed the users/ericsciple/m167retry branch from 6797a48 to 728942f Compare March 9, 2020 17:06

@thboop thboop left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ericsciple
ericsciple force-pushed the users/ericsciple/m167retry branch from 728942f to abde3c6 Compare March 9, 2020 18:25
@ericsciple
ericsciple force-pushed the users/ericsciple/m167retry branch from abde3c6 to 3dedf1c Compare March 9, 2020 18:28
@ericsciple
ericsciple merged commit 5859d71 into master Mar 9, 2020
@ericsciple
ericsciple deleted the users/ericsciple/m167retry branch March 9, 2020 18:35

@immense055 immense055 left a comment

Copy link
Copy Markdown

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 () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to learn

Derrick-MUGISHA pushed a commit to Derrick-MUGISHA/toolkit that referenced this pull request Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants