Copilot commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4125495377
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +811,67 @@ impl FileWrite for OpenDalWriter {
mod tests {
use super::*;
+ fn client_config(value: &str) -> Result<OpenDalClientConfig> {
+ OpenDalClientConfig::from_properties(&HashMap::from([(
+ OPENDAL_IO_TIMEOUT_MS.to_string(),
+ value.to_string(),
+ )]))
+ }
+
+ #[test]
+ fn test_io_timeout_parsing() {
+ let unset =
OpenDalClientConfig::from_properties(&HashMap::new()).unwrap();
+ assert_eq!(unset.io_timeout_ms(), DEFAULT_IO_TIMEOUT_MS);
+ assert_eq!(client_config("45000").unwrap().io_timeout_ms(), 45_000);
+
+ for invalid in ["0", "-1", "12.5", "abc", ""] {
+ let err = client_config(invalid).unwrap_err().to_string();
+ assert!(err.contains(OPENDAL_IO_TIMEOUT_MS), "{invalid}");
+ assert!(err.contains(&format!("value: {invalid:?}")), "{err}");
+ }
+ }
+
+ #[test]
+ fn test_default_io_timeout_matches_opendal() {
+ // `TimeoutLayer` has no getters, so compare through `Debug`. An
OpenDAL upgrade that
+ // changes its default fails here instead of silently diverging from
it.
+ assert_eq!(
+ format!("{:?}", TimeoutLayer::new()),
+ format!(
+ "{:?}",
+
TimeoutLayer::new().with_io_timeout(Duration::from_millis(DEFAULT_IO_TIMEOUT_MS))
+ ),
+ );
Review Comment:
This test relies on `Debug` output for semantic equivalence, which is
brittle: `Debug` formatting can change without any behavior/default change,
producing false failures. A more stable approach is to assert the constant
value you intend to track (e.g., `DEFAULT_IO_TIMEOUT_MS == 10_000`) and
document that it should be updated if OpenDAL’s default changes, or gate the
`Debug` comparison behind a less strict check (e.g., only when OpenDAL versions
are pinned) to reduce spurious CI breakage.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +102,55 @@ cfg_if! {
mod resolving;
pub use resolving::{OpenDalResolvingStorage, OpenDalResolvingStorageFactory};
+/// Deadline in milliseconds for one IO operation, and for every method call
on a returned
+/// reader, writer, lister or deleter. Honored by every [`OpenDalStorage`]
backend, where it
+/// defaults to 10000 to match OpenDAL's `TimeoutLayer`.
Review Comment:
The doc comment hard-codes the default as `10000`, but the actual default is
controlled by `DEFAULT_IO_TIMEOUT_MS`. This can easily drift if the constant
changes. Prefer referencing `DEFAULT_IO_TIMEOUT_MS` (or wording like “defaults
to OpenDAL’s `TimeoutLayer` default (currently 10s)”) to avoid duplicating a
magic number in docs.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +476,47 @@ 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(Duration::from_millis(self.client().io_timeout_ms())),
+ )
+ .layer(RetryLayer::new());
Review Comment:
The relative ordering of `TimeoutLayer` vs `RetryLayer` is behaviorally
significant (per-attempt timeout vs total-across-retries). Since this is subtle
and easy to accidentally change during refactors, add a short inline comment
here stating the intended wrapping order (e.g., that `RetryLayer` must wrap
`TimeoutLayer` so each retry attempt gets its own timeout budget).
--
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]
