-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core] rate limit handling on online file source #6223
Conversation
I've implemented the core and platform specifics. All platforms, expect for Android, have the same unit test. It would be nice to also test this against an actual server with a low rate limit as well, just to be sure.
|
Error(Reason, std::string = "", optional<RetryAfter> = {}); | ||
|
||
private: | ||
optional<Timestamp> parseRetryAfter(const optional<RetryAfter> retryAfter_) const; |
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.
Can you move this to http_header.hpp
and make the signature:
optional<Timestamp> parseRetryHeaders(const optional<std::string>& retryAfter,
const optional<std::string>& xRateLimitReset);
Advantages:
- No need for
RetryAfter
type - Factor conditional and integer conversion logic from individual SDK implementations
@jfirebaugh Thanks for taking a look. I've changed the implementations and will rework the tests. |
df09d00
to
1eebf71
Compare
When writing the unit tests I ran into a little issue where the the timeout became negative on high number of errors as the sign bit flipped (here among others). Changed all three exponential backoffs so that doesn't happen ( |
|
||
optional<Timestamp> parseRetryHeaders(const optional<std::string>& retryAfter, | ||
const optional<std::string>& xRateLimitReset) { | ||
optional<Timestamp> result; |
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.
Prefer early returns to temporary variables:
if (retryAfter) {
try {
return ...;
} catch ... {
return ...;
}
}
if (xRateLimitReset) {
return ...;
}
return {};
71dd26b
to
706e93b
Compare
|
||
optional<Timestamp> parseRetryHeaders(const optional<std::string>& retryAfter, | ||
const optional<std::string>& xRateLimitReset) { | ||
if(retryAfter) { |
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.
nit, missing space after if
Thanks for adding more tests! |
706e93b
to
015e942
Compare
@tmpsantos Thanks for the review! Made the changes as suggested. Waiting on CI and then I'll merge. |
015e942
to
ed4e37b
Compare
Fixes #5821