[CHORE] Classify throttle and internal errors as Retryable in Python #2914
Conversation
CodSpeed Performance ReportMerging #2914 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2914 +/- ##
==========================================
- Coverage 78.34% 75.33% -3.02%
==========================================
Files 596 596
Lines 69875 70952 +1077
==========================================
- Hits 54747 53454 -1293
- Misses 15128 17498 +2370
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| "SlowDown", | ||
| "PriorRequestNotComplete", | ||
| "EC2ThrottledException", | ||
| ]; |
There was a problem hiding this comment.
Where did this come from? Should we document this list somehow
src/daft-io/src/s3_like.rs
Outdated
| @@ -140,24 +159,33 @@ impl From<Error> for super::Error { | |||
| source: source.into(), | |||
| } | |||
| } else { | |||
| super::Error::UnableToOpenFile { | |||
| super::Error::MiscTransient { | |||
There was a problem hiding this comment.
Can we add some docs here making it explicit that this is an explicit decision to classify all other dispatch errors as "who knows what happened here, let's just tell the user it's transient"?
src/daft-io/src/s3_like.rs
Outdated
| err => match err.code() { | ||
| Some("InternalError") => super::Error::MiscTransient { | ||
| path, | ||
| source: err.into(), | ||
| }, | ||
| Some(code) if THROTTLING_ERRORS.contains(&code) => { | ||
| super::Error::Throttled { | ||
| path, | ||
| source: err.into(), | ||
| } | ||
| } | ||
| _ => super::Error::Unhandled { | ||
| path, | ||
| msg: DisplayErrorContext(err).to_string(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Seems like this group of logic is being repeated in 3 different places. Any chance that we could extract it into its own function?
No description provided.