blackmwk commented on code in PR #2455:
URL: https://github.com/apache/iceberg-rust/pull/2455#discussion_r3332380260
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -326,9 +326,17 @@ impl OpenDalStorage {
}
};
- // Transient errors are common for object stores; however there's no
- // harm in retrying temporary failures for other storage backends as
well.
- let operator = operator.layer(RetryLayer::new());
+ // Apply observability/resilience layers. TimeoutLayer must be
+ // inside RetryLayer so each retry attempt is independently
+ // bounded — without a per-attempt timeout, a future parked on a
+ // silently dropped TCP connection never produces an `Err` and
+ // RetryLayer cannot retry, leaving the caller hung indefinitely.
+ // See:
https://opendal.apache.org/docs/rust/opendal/layers/struct.TimeoutLayer.html
+ //
+ // Transient errors are common for object stores; we retry temporary
+ // failures with exponential backoff. The retry behavior also
+ // benefits non-object-store backends.
+ let operator =
operator.layer(TimeoutLayer::new()).layer(RetryLayer::new());
Review Comment:
I think we should honor configurations for different backend storages to
build the opendal, and simply adding a timeout layer doesn't help too much.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
