-
Notifications
You must be signed in to change notification settings - Fork 95
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: support HTTP 429 with Retry-After #194
Conversation
Codecov Report
@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 29.75% 30.01% +0.25%
==========================================
Files 100 101 +1
Lines 11268 11312 +44
==========================================
+ Hits 3353 3395 +42
- Misses 7547 7549 +2
Partials 368 368
|
Makes it possible to pass RetryAfter as a hint for any error, not just 429
First stab at making things more generic
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.
💭 something I've realized during this review is that Retry-After
is not limited to 429 Too Many Requests, which is used when user is sending too many requests. There are cases where Retry-After
is returned with 503 Service Unavailable (when it is not specific to user or their IP, could be global outage).
Pushed two commits which replace all standalone errors with generic wrappers: ErrorResponse
and ErrorRetryAfter
, and attempted to wire that up in ipfs-inactive/bifrost-gateway#56 – existing tests pass and does the trick, but I feel we could simplify things somehow.
@hacdias lmk if you have any idea how to make this better (if not, we can merge as-is, to unblock Retry-After
, and refine later)
@lidel I think the way you rewrote it is a good idea. I'm not sure how we could simplify this now, but maybe later we will figure something out. Merged for now. |
This closes #188.
errors.go
. I also added some simply sanity tests for this new error as it is its own custom type to support theRetryAfter
field.gateway.ErrTooManyRequests
. Returning an error of this type will make the gateway return 429. If the fieldRetryAfter
above 0, it will set theRetry-After
header. Also added a more "e2e" test for this, albeit arguable unnecessary since I added tests forwebError
.