Kurtiscwright commented on code in PR #2455:
URL: https://github.com/apache/iceberg-rust/pull/2455#discussion_r3327200232
##########
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 its reasonable to follow up with fine grained configurations and
merging as is fine. I would request (and I can handle this if your good handing
off ownership) that the fine grained configurations are done at the storage
trait layer.
I recommended this PR for merging and inclusion into the 0.10 release being
gathered here:
- https://github.com/apache/iceberg-rust/issues/2527
--
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]
