kszlim commented on code in PR #5383:
URL: https://github.com/apache/arrow-rs/pull/5383#discussion_r1485686244
##########
object_store/src/client/retry.rs:
##########
@@ -263,7 +262,7 @@ impl RetryExt for reqwest::RequestBuilder {
do_retry = true
} else if let Some(source) = e.source() {
if let Some(e) =
source.downcast_ref::<hyper::Error>() {
- if e.is_connect() || e.is_closed() ||
e.is_incomplete_message() {
+ if !(e.is_parse() || e.is_parse_status() ||
e.is_parse_too_large() || e.is_user() || e.is_canceled()) {
Review Comment:
Anyways what are your thoughts on the code in this PR? I do agree this would
be more aggressive than needed vs what exists which is probably more
conservative than what is "perfect". I definitely think the best way forward is
to have hyper expose more information about the failure and then let object
store use that to perform more fine grained retries.
But as is, I personally think having more aggressive retries is better than
more conservative retries, but that's definitely a matter of opinion. My
reasoning is that users can always tune down the back off/retries/timeout
(maybe it'd be useful to have a global retry timeout for a single request, ie.
there exists one for attempt and then a longer one for all the associated
attempts added together?), but if object store doesn't retry on some genuinely
safe to retry on cases, the user has no recourse and must retry on top of
object store (with less direct context to work with).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
