comphead opened a new pull request, #3263: URL: https://github.com/apache/iceberg-rust/pull/3263
## Which issue does this PR close? - Part of #2977. ## What changes are included in this PR? `iceberg-storage-opendal` wraps every FileIO operator in `TimeoutLayer::new()`. Its 10s `io_timeout` bounds each `read`/`write` and **every method call on a returned reader, writer, lister or deleter**, and nothing in the crate's property or builder surface can override it: https://github.com/apache/iceberg-rust/blob/bb1e4a4/crates/storage/opendal/src/lib.rs#L394 Any operation that legitimately needs longer than 10s fails *deterministically*, not flakily: `RetryLayer` re-sends the same request, which cannot fit in the budget either, so all four attempts die at the same place and the error surfaces as persistent. ``` Unexpected (persistent) at read, context: { timeout: 10 } => io operation timeout reached ``` #3179 bounds the S3 *write* request size, which removes the oversized-part case on the write path. This PR covers the general one, including reads, by making the budget itself configurable: - Add `CLIENT_IO_TIMEOUT_MS` (`client.io-timeout-ms`), parsed in `utils.rs` and handed to `TimeoutLayer::with_io_timeout`. It sits in the existing `client.*` namespace next to `client.region`, so one property covers every backend rather than one per service. - Unset keeps OpenDAL's 10s, so behaviour is unchanged by default. Non-numeric and zero are rejected at `build` time rather than silently ignored, since zero would time every operation out before it starts. - `TimeoutLayer` stays inside `RetryLayer`, so each attempt is still independently bounded. Note this does not scale the deadline with payload size: OpenDAL dropped `TimeoutLayer::with_speed` in apache/opendal#6793, so option 2 in #2977 is no longer available. The `timeout` budget for control operations (`stat`, `rename`, `presign`, default 60s) is left alone, as it has not been reported as a problem. ## Are these changes tested? Unit tests: - `io_timeout_ms_parse`: default when unset, override, and rejection of `0`, `-1`, `12.5`, `abc`, `""`. - `OpenDalStorage::io_timeout` returns the configured value, and the OpenDAL default when unset. - `OpenDalStorageFactory::build` surfaces a parse failure instead of falling back. - `OpenDalResolvingStorage::resolve` propagates the property into the storage it builds. Asserting that the layer actually fires at the configured deadline needs a backend that stalls on demand, which the current integration suite has no fixture for. The seam covered here is the value reaching `TimeoutLayer::with_io_timeout`. ## AI Disclosure - AI-assisted implementation. -- 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]
