feat(storage-opendal)!: make the per-IO-operation timeout configurable by comphead · Pull Request #3263 · apache/iceberg-rust

comphead · 2026-09-22T16:40:51Z

Which issue does this PR close?

Part of opendal FileIO hard-wires TimeoutLayer defaults (10s per-IO) with no way to configure them; large Parquet row groups fail deterministically on S3 #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 property or builder surface can override it:

https://github.com/apache/iceberg-rust/blob/bb1e4a4/crates/storage/opendal/src/lib.rs#L394

An operation that legitimately needs longer then fails deterministically, not flakily: RetryLayer re-sends the same request, which cannot fit in the budget either, so every attempt dies at the same place and the error surfaces as persistent.

Unexpected (persistent) at read, context: { timeout: 10 } => io operation timeout reached

Add client.io-timeout-ms and hand it 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. Zero and non-numeric values are rejected at build time rather than silently ignored. TimeoutLayer stays inside RetryLayer, so each attempt is still independently bounded.

#3179 bounds the S3 write request size, which removes the oversized-part case on the write path; this covers the general one, including reads. Scaling the deadline with payload size (option 2 in #2977) is not available: OpenDAL dropped TimeoutLayer::with_speed in apache/opendal#6793.

The 60s timeout for control operations 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, and the empty string.
OpenDalStorageFactory::build surfaces a parse failure instead of falling back.
OpenDalResolvingStorage::resolve propagates the property into the storage it builds.

Asserting the layer actually fires at the configured deadline needs a backend that stalls on demand, which the integration suite has no fixture for. What is covered is the value reaching TimeoutLayer::with_io_timeout.

AI Disclosure

AI-assisted implementation.

comphead · 2026-09-22T16:46:40Z

@mbutrovich FYI

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 3 · 1

Open (4)

The wildcard arm (_ => default_io_timeout_ms()) plus #[allow(unreachable_patterns)] makes this… · New
Hardcoding 10_000 as the OpenDAL TimeoutLayer default is brittle: if OpenDAL changes its… · New
For values like the empty string, the error renders as Invalid client.io-timeout-ms: , ..., which… · New
The tests duplicate the default value (10_000). Using default_io_timeout_ms() for the expected… · New

What changed in this PR

Adds a new storage configuration property to make OpenDAL’s per-IO-operation timeout configurable, and wires it through the OpenDAL storage factory/resolver into TimeoutLayer.

Changes:

Introduce client.io-timeout-ms (CLIENT_IO_TIMEOUT_MS) as a shared storage property in iceberg.
Parse/validate the timeout in iceberg-storage-opendal and propagate it through factory + resolving storage variants.
Apply the configured timeout to TimeoutLayer::with_io_timeout, plus add unit tests for parsing and propagation.

File	Description
crates/storage/opendal/src/utils.rs	Adds default + parsing/validation for `client.io-timeout-ms` and unit tests.
crates/storage/opendal/src/resolving.rs	Parses timeout once per build and stores it in each resolved `OpenDalStorage` variant; adds propagation test.
crates/storage/opendal/src/lib.rs	Extends `OpenDalStorage` variants to carry `io_timeout_ms`, parses it in `StorageFactory::build`, and applies it to `TimeoutLayer`.
crates/storage/opendal/public-api.txt	Updates exported API surface to reflect enum variant shape/fields changes.
crates/iceberg/src/io/storage/config/mod.rs	Introduces the `CLIENT_IO_TIMEOUT_MS` property constant with docs.
crates/iceberg/public-api.txt	Public API snapshot updated to include `CLIENT_IO_TIMEOUT_MS`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

        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(),


+/// Matches the `opendal::layers::TimeoutLayer` default.
+pub(crate) fn default_io_timeout_ms() -> u64 {
+    10_000
+}


+        _ => Err(iceberg::Error::new(
+            iceberg::ErrorKind::DataInvalid,
+            format!("Invalid {CLIENT_IO_TIMEOUT_MS}: {value}, expected a positive integer"),
+        )),


+
+    #[test]
+    fn test_io_timeout_ms_parse() {
+        assert_eq!(io_timeout_ms_parse(&HashMap::new()).unwrap(), 10_000);


xanderbailey · 2026-09-22T20:07:01Z

 impl StorageFactory for OpenDalStorageFactory {
    #[allow(unused_variables)]
    fn build(&self, config: &StorageConfig) -> Result<Arc<dyn Storage>> {
+        let io_timeout_ms = io_timeout_ms_parse(config.props())?;


Do you think it would be better to have an OpenDalClientConfig struct? So that we could add retry configuration etc here also?

I'd go with a struct too, and build it with #[derive(Properties)]. @blackmwk asked for the derive on the new HDFS config in #3111 (comment), and #3094 is moving the catalog configs over to it (see SqlCatalogProperties). Something like this:

const DEFAULT_IO_TIMEOUT: Duration = Duration::from_secs(10); #[derive(Clone, Debug, Properties, Serialize, Deserialize)] pub struct OpenDalClientConfig { /// Per-attempt deadline for one IO operation. #[property( key = CLIENT_IO_TIMEOUT_MS, default = DEFAULT_IO_TIMEOUT, parse_with = parse_io_timeout, getter )] io_timeout: Duration, }

from_properties handles the default and adds the property key to the error context. That leaves parse_io_timeout responsible only for rejecting zero and non-integers, and default_io_timeout_ms() becomes a named const. I compiled this shape against the head commit. Unset gives 10s, 45000 gives 45s, and 0, "", and abc all fail with DataInvalid, context: { property: client.io-timeout-ms }. It also round-trips through serde, which the FileIO serialization path needs. The crate would need an iceberg-property-macro dependency, the same way iceberg-catalog-sql has one.

Each variant would then carry client: OpenDalClientConfig in place of a bare io_timeout_ms: u64. This PR already changes every variant of the public OpenDalStorage enum (see public-api.txt). With a struct, adding retry or control-timeout settings later won't change the enum again. Keeping io_timeout private behind the generated getter also avoids adding another pub field.

xanderbailey

Thanks for working on this! I do think having a more generic OpenDal config might work better here and we can parse it with the new properties macro. It'll make this more expandable in the future I think. WDYT?

mbutrovich

Thanks @comphead for picking this up, Comet will be glad to have it. My comments are about how the new setting fits the repo's config conventions: how it gets parsed and how it's carried on the OpenDalStorage variants.

mbutrovich · 2026-09-22T20:50:38Z

+/// 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.


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?

Suggested change

/// 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.

/// 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`.

mbutrovich · 2026-09-22T20:51:02Z

+    #[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)


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). 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.

`iceberg-storage-opendal` wraps every FileIO operator in `TimeoutLayer::new()`, whose 10s `io_timeout` bounds each `read`/`write` and each method call on a returned reader or writer. Nothing in the property or builder surface can override it, so an operation that legitimately needs longer fails deterministically: the `RetryLayer` above re-sends the same request, which cannot fit in the budget either. Add `client.io-timeout-ms`, parsed into an `OpenDalClientConfig` built with `#[derive(Properties)]` and handed to `TimeoutLayer::with_io_timeout`. The config's fields are private, so later client settings such as retry are additive rather than breaking. The default is unchanged. BREAKING CHANGE: `OpenDalStorage` variants carry a `client` field, and `OpenDalStorage::Memory` is now a struct variant.

Copilot AI lite review requested due to automatic review settings September 22, 2026 16:40

comphead force-pushed the io-timeout-config branch from 43cc854 to b1751f9 Compare September 22, 2026 16:45

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Copilot started reviewing on behalf of comphead September 22, 2026 17:59 View session

xanderbailey reviewed Sep 22, 2026

View reviewed changes

mbutrovich reviewed Sep 22, 2026

View reviewed changes

comphead mentioned this pull request Sep 22, 2026

Iceberg scan fails with a hard-wired 10s OpenDAL io_timeout that cannot be configured apache/datafusion-comet#6124

Open

comphead force-pushed the io-timeout-config branch from b1751f9 to 0c255ee Compare September 23, 2026 03:36

comphead force-pushed the io-timeout-config branch from 0c255ee to 1ef4025 Compare September 23, 2026 03:43

Navigation Menu

feat(storage-opendal)!: make the per-IO-operation timeout configurable - #3263

feat(storage-opendal)!: make the per-IO-operation timeout configurable#3263
comphead wants to merge 1 commit into
apache:mainfrom
comphead:io-timeout-config

comphead commented Sep 22, 2026 •

edited

Loading

Uh oh!

comphead commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

xanderbailey Sep 22, 2026

Uh oh!

mbutrovich Sep 22, 2026

Uh oh!

xanderbailey left a comment

Uh oh!

mbutrovich left a comment

Uh oh!

mbutrovich Sep 22, 2026

Uh oh!

mbutrovich Sep 22, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		/// 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.

Conversation

comphead commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

comphead commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

xanderbailey Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

xanderbailey left a comment

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

mbutrovich Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

comphead commented Sep 22, 2026 •

edited

Loading