mbutrovich commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4076366705
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +426,35 @@ impl OpenDalStorage {
// 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());
+ let operator = operator
+ .layer(TimeoutLayer::new().with_io_timeout(self.io_timeout()))
+ .layer(RetryLayer::new());
Ok((operator, relative_path))
}
+ /// Per-IO-operation deadline, from
[`CLIENT_IO_TIMEOUT_MS`](iceberg::io::CLIENT_IO_TIMEOUT_MS).
+ #[allow(unreachable_patterns)]
+ fn io_timeout(&self) -> Duration {
+ let ms = match self {
+ #[cfg(feature = "opendal-memory")]
+ OpenDalStorage::Memory { io_timeout_ms, .. } => *io_timeout_ms,
+ #[cfg(feature = "opendal-fs")]
+ OpenDalStorage::LocalFs { io_timeout_ms } => *io_timeout_ms,
+ #[cfg(feature = "opendal-s3")]
+ OpenDalStorage::S3 { io_timeout_ms, .. } => *io_timeout_ms,
+ #[cfg(feature = "opendal-gcs")]
+ OpenDalStorage::Gcs { io_timeout_ms, .. } => *io_timeout_ms,
+ #[cfg(feature = "opendal-oss")]
+ OpenDalStorage::Oss { io_timeout_ms, .. } => *io_timeout_ms,
+ #[cfg(feature = "opendal-azdls")]
+ OpenDalStorage::Azdls { io_timeout_ms, .. } => *io_timeout_ms,
+ #[cfg(feature = "opendal-hf")]
+ OpenDalStorage::Hf { io_timeout_ms, .. } => *io_timeout_ms,
+ _ => default_io_timeout_ms(),
+ };
+ Duration::from_millis(ms)
Review Comment:
With `#[allow(unreachable_patterns)]` and the `_ => default_io_timeout_ms()`
arm, this match always compiles. If a new variant lands without an arm here
(the HDFS variant in #3111 is one), it compiles and silently ignores the user's
timeout. `create_operator` handles the same no-backend case by gating its `_`
arm with `#[cfg(all(not(feature = "opendal-s3"), ...))]` ([lines
403-416](https://github.com/apache/iceberg-rust/blob/b1751f978f2ed8437e8f0c65d4d0564748ea8df4/crates/storage/opendal/src/lib.rs#L403-L416)).
Could this use the same gate, with `opendal-memory` added to the list, and
drop the `allow`? Then a missing arm is a compile error. This applies equally
if the method ends up returning `&OpenDalClientConfig`.
##########
crates/iceberg/src/io/storage/config/mod.rs:
##########
@@ -45,6 +45,12 @@ pub use oss::*;
pub use s3::*;
use serde::{Deserialize, Serialize};
+/// Deadline in milliseconds for one IO operation, and for every method call
on a returned
+/// reader, writer, lister or deleter. Applies to all backends. Defaults to
10000.
Review Comment:
Only `iceberg-storage-opendal` reads this key. A third-party
`StorageFactory` that gets these props won't honor it, so "Applies to all
backends" could mislead someone configuring a different storage. The 10000
default is also OpenDAL's value, not a property of the key. Could the doc say
where it applies?
```suggestion
/// Deadline in milliseconds for one IO operation, and for every method call
on a returned
/// reader, writer, lister or deleter. Honored by every
`iceberg-storage-opendal` backend, where it defaults to 10000 to match
OpenDAL's `TimeoutLayer`.
```
--
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]
