-
Notifications
You must be signed in to change notification settings - Fork 236
fix: add retry to getting GCS client config #3930
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3930 +/- ##
==========================================
- Coverage 78.52% 78.52% -0.01%
==========================================
Files 772 773 +1
Lines 98313 98366 +53
==========================================
+ Hits 77204 77237 +33
- Misses 21109 21129 +20
🚀 New features to boost your workflow:
|
src/daft-io/src/google_cloud.rs
Outdated
HttpError(reqwest_err) | ||
if reqwest_err | ||
.status() | ||
.is_some_and(|status| status.as_u16() == 401) => |
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.
So my understanding is that the 401
actually comes from when this fails, the client goes into anon mode and then does a request to GCS without credentials. That error is the 401.
Based off the user logs, the credential error can actually be some other http error (pretty sure its not 401)
(ScanWithTask-FanoutEvenSlices [Stage:2] pid=2781, ip=10.68.5.21) Google Cloud Storage Credentials not provided or found when making client. Reverting to Anonymous mode.
(ScanWithTask-FanoutEvenSlices [Stage:2] pid=2781, ip=10.68.5.21) Details
(ScanWithTask-FanoutEvenSlices [Stage:2] pid=2781, ip=10.68.5.21) Unable to load Credentials: http error: error sending request for url (https://oauth2.googleapis.com/token)
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.
Hm should we just retry on any HTTP error 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.
Okay so I think we want to retry when the reqwest error type is Kind::Request
or if the error code is the standard retry able stuff like 429 or 5XX
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 like this should work
HttpError(reqwest_err)
if reqwest_err.is_request() || reqwest_err.is_timeout() || matches!(reqwest_err.status().map(|s| s.as_u16()), Some(429) | Some(500..599)) =>
Also contains a tiny refactor to centralize logic for exponential backoff retry
Also contains a tiny refactor to centralize logic for exponential backoff retry