feat!: Add automatic retrying of 429/502/503 #199
Conversation
There was a problem hiding this comment.
could you change the PR title to: feat!: add automatic retry on 429
Just so that semantic release bumps the right version and generates appropriate release notes. We can do this major release together with the useCdn one so we don't have two breaking releases in short succession
judofyr
force-pushed
the
automatic-retry
branch
from
998c383
to
c726e03
Compare
It's enough with the PR title? Or should the commit also have the |
|
Commit title as well
…On Tue, 18 Apr 2023 at 14:13, Magnus Holm ***@***.***> wrote:
could you change the PR title to: feat!: add automatic retry on 429
It's enough with the PR title? Or should the commit also have the !?
—
Reply to this email directly, view it on GitHub
<#199 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUAPL6WC4DZTMESFWKY6TXB2AN5ANCNFSM6AAAAAAXCP5QFQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
judofyr
force-pushed
the
automatic-retry
branch
from
c726e03
to
dc90218
Compare
judofyr
changed the title
|
There was a problem hiding this comment.
-
This looks great - will be really nice to get those temporary errors retried. I think this is unlikely to "solve" rate limiting (with such low first-retry timeouts), but also probably won't make it worse. I Have A Branch™ that implements actual rate-limiting that I'd like to finish up at some point, but we can leave that for a later time.
-
Maybe we should provide a way to disable the retrying? Passing
{maxRetries: 0}will actually end up setting it to5currently (unfortunate choice, I guess we might want to fix this in get-it). -
I would prefer if we also added an explicit
BREAKING CHANGE:explanation to the commit(s), in order for explain it in a little more detail for automatic release notes. For instance:
BREAKING CHANGE: Client will now automatically retry all GET/HEAD requests as
well as queries if the server responds with a 429, 502 or 503 status code - as
well as on socket/dns errors. Previously, the client would immediately throw
an error. If you have application-level retry code, you should either disable
the retrying in the client by passing `{maxRetries: 0}`, or remove the custom
retry code and potentially alter the `shouldRetry` and `maxRetries` options to
match your wanted behavior.
1b07301
to
6880c60
Compare
|
Okay, I've made the following changes:
I'd prefer to merge this now and then rather implement "retry also on DNS errors for POST requests" in a later (patch) release as I think this is best solved by adding support for it in |
BREAKING CHANGE: Client will now automatically retry all GET/HEAD
requests as well as queries if the server responds with a 429, 502 or
503 status code - as well as on socket/dns errors. Previously, the
client would immediately throw an error. If you have application-level
retry code, you should either disable the retrying in the client
by passing `{maxRetries: 0}`, or remove the custom retry code and
potentially alter the `retryDelay` and `maxRetries` options to match
your wanted behavior.
judofyr
force-pushed
the
automatic-retry
branch
from
6880c60
to
ff3a006
Compare
| const httpRequest = defineHttpRequest(envMiddleware, {}) | ||
| /** @public */ | ||
| export const requester = httpRequest.defaultRequester |
There was a problem hiding this comment.
Out of scope for this PR, but it does seem like we should export defineHttpRequest instead of requester. I'm not sure what use there is for requester anymore. What do you think @rexxars?
There was a problem hiding this comment.
There are a few usages internally, but I'm sure they could be done differently: https://github.com/search?q=org%3Asanity-io+requester&type=code
This is a bit of a bigger, and technically breaking, change which introduces automatic retrying of 429 errors (i.e. rate limited). I'm opening it here to start a discussion about how to progress this forward. I believe this would be very useful for the majority of the users of the client and should be enabled by default.