Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(node): add exponential retry mechanism with idempotency headers #4462
feat(node): add exponential retry mechanism with idempotency headers #4462
Changes from 3 commits
41d77fd
b66965a
503257c
7475e22
6670e32
41296c2
071d59f
1323c04
d70b9f8
bf68b2d
3474d5a
a3e150d
2d548b4
208d803
bbcbb80
024d8cd
60a000d
8479ad0
b773f3a
01649c2
fe525ae
c75140c
1e8c1b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
without that change, tests for retries don't work.
I observed strange behavior - it sends GET request to
127.0.0.1:80
even when you send POST request to baseUrl, it messes up the request config.change to
^axios$
fixed that issue. This is also recommendation inside nock readme for axios.I also tested manually using symlink in testing repo's
node_modules
and retries work as expected.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.
This test is great. Could we add another test using the same format with
times(4)
and verify that a500
is received?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.
Nice test 🙌
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'm using axios interceptor to attach idempotency key header to each request.
Idempotency key is attached only to
POST
andPATCH
methods because they are not idempotent by definition.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.
isNetworkError
is imported fromaxios-retry
library.As we know HTTP is based on TCP/IP protocol and sometimes we facing issues on this network level.
TCP/IP error typically contains some
code
like"ECONNRESET"
andisNetworkError
uses under the hood library is-retry-allowed, because not every network error should be retried.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.
It's not clear why we're retrying when a response is unavailable. Could you please shed some light on this one?
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 it's mistake (should return
false
) - if statements after this check are dependent onerr.response
(possibly undefined). In new changes I made it look cleaner.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.
On HTTP level, I decided to retry on every
5xx
error,408
(Request Timeout) and429
(Too Many Requests)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.
You should also retry on 422 but regen the idempotency key