-
Notifications
You must be signed in to change notification settings - Fork 3k
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
cvat-core: retry HTTP requests if a 429 status is returned #7216
Conversation
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.
We also have a worker cvat-core/src/download.worker.ts
to download data chunks in another thread
Will this plugin have effect inside the worker?
cvat-core/src/server-proxy.ts
Outdated
retryDelay: (retryCount, error) => { | ||
const retryAfterValue = error.response.headers['retry-after']; | ||
|
||
// Retry-After is allowed to be a number as well as a date. | ||
// We're probably not going to use the latter option, though, so don't bother parsing it. | ||
if (!retryAfterValue || !/^\d+$/.test(retryAfterValue)) return axiosRetry.exponentialDelay(retryCount, error); | ||
|
||
return parseInt(retryAfterValue, 10) * 1000; | ||
}, |
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.
If error.response.headers['retry-after']
is supposed to be a number or not a number string:
retryDelay: (retryCount, error) => { | |
const retryAfterValue = error.response.headers['retry-after']; | |
// Retry-After is allowed to be a number as well as a date. | |
// We're probably not going to use the latter option, though, so don't bother parsing it. | |
if (!retryAfterValue || !/^\d+$/.test(retryAfterValue)) return axiosRetry.exponentialDelay(retryCount, error); | |
return parseInt(retryAfterValue, 10) * 1000; | |
}, | |
retryDelay: (retryCount: number, error: Error): number => { | |
// Retry-After is allowed to be a number as well as a date. | |
// We're probably not going to use the latter option, though, so don't bother parsing it. | |
const retryTimeout = (+error.response.headers['retry-after']) * 1000; | |
if (Number.isNaN(retryTimeout)) { | |
return axiosRetry.exponentialDelay(retryCount, error); | |
} | |
return retryTimeout; | |
}, |
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.
By "number" I really mean "non-negative decimal integer". See the definition of Retry-After
.
Blindly converting to a JavaScript number could yield undesirable values like negative numbers or infinity, so I'd rather not do that.
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.
For avoid infinity/floats we may use:
Number.isInteger(retryTimeout) && retryTimeout > 0
Instead of isNaN
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 is a little bit more pretty, than regex using
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 is a little bit more pretty, than regex using
I don't know about pretty, but I find my version more straightforward. I verify that:
- The header is present.
- The header value follows the expected grammar.
Then I convert the value to the expected type.
Your version relies on JS's numeric conversion doing the right thing for values not matching the grammar, which isn't even true:
> +'0xA'
10
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.
Why not true? You just converted HEX 0xA to decimal 10.
And this is correct.
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.
Let's check with regex any data come from the server then...?
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.
Why not true? You just converted HEX 0xA to decimal 10.
And this is correct.
It's not correct, because hexadecimal is not allowed by the grammar.
Let's check with regex any data come from the server then...?
Sure? If it's amenable to such validation, then why not.
What's your point? Plus signs are not allowed by the grammar, so this string should be rejected - and that's what my implementation does.
But we do not expect
+55
from the server, right? As well as0xA
..
We don't - and that's why I reject such values.
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.
My point is that your example with 0xA is not real. If server returns '0xA' - server does not work correctly.
As if server returns '+55' - it is valid number, but the regular experssion rejects it. But this value also is not real.
IMHO we should not use regex to validate numbers, it looks ugly in the code.
It is up to you how implement, you asked for a review, I reviewed.
But to be honest, looking at the worker now, I do not remember why it was added. |
Well, you tell me if you want me to add retry code to the worker. 🤷♂️ |
First of all need to check that this retry logic has effect inside the worker, if it does not, so, please add the code there. |
Okay, I'll do that. (FWIW, I'm pretty sure the current code will not affect the worker, since the worker doesn't import |
This should help reduce breakage if a user hits an API rate limit.
I updated the code to share basic configuration (including retries) between the worker and |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7216 +/- ##
===========================================
- Coverage 81.54% 81.31% -0.23%
===========================================
Files 365 367 +2
Lines 39261 39364 +103
Branches 3631 3646 +15
===========================================
- Hits 32016 32010 -6
- Misses 7245 7354 +109
|
Motivation and context
This should help reduce breakage if a user hits an API rate limit.
How has this been tested?
Manual testing.
Checklist
develop
branch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.