You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2026/09/22 16:40:54 UTC
[PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5981913452
@comphead if you’re happy with the changes, lmk and I can press merge!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5930562102
Not 100% sure about the serde break being an issue here. My understanding of these structs being serde is not for the purpose of persisting them but rather for distributed engines to be able to send them over the wire. Now granted you could argue that we don’t have control over that but I think this would be a misuse no? Probably something we should document in the crate?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5818316666
Depends on https://github.com/apache/iceberg-rust/pull/3263
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5981909980
Thanks @kevinjqliu appreciate if we can merge the PR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5834429915
@kevinjqliu cc
appreciate if you can take a look, in Comet we got file scan tasks restarted because of uncontrollable timeout which makes the entire execution time longer. the PR is to expose timeout value to external users
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5935577551
Thanks @laskoviymishka and @xanderbailey for the review, PTAL
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4075965831
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -163,35 +164,42 @@ where
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())?;
Review Comment:
Do you think it would be better to have an `OpenDalClientConfig` struct? So that we could add retry configuration etc here also?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4157595501
##########
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:
Kept it, since `TimeoutLayer` has no getters and both sides go through the same derived `Debug`, so a format change alone cannot make them differ. The `assert_ne!` from 5e9641c catches a `Debug` that stops printing `io_timeout`.
##########
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:
Fixed. The doc now links to the new `OPENDAL_IO_TIMEOUT_MS_DEFAULT` instead of repeating the number.
##########
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 comment just above this hunk, unchanged from `main`, already says `TimeoutLayer` must be inside `RetryLayer` so each retry attempt is independently bounded.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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:
Done. The default is now a public `OPENDAL_IO_TIMEOUT_MS_DEFAULT: u64`, following the `*_DEFAULT` consts in `iceberg`, and the key's doc links to it instead of repeating the number.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Matches the `opendal::layers::TimeoutLayer` default.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(10_000).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms,
+ getter
+ )]
+ io_timeout_ms: NonZeroU64,
Review Comment:
Went with the `Duration` accessor. The generated getter is gone, and `OpenDalClientConfig::io_timeout()` returns a `Duration` by value, so the millisecond `NonZeroU64` stays private.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Matches the `opendal::layers::TimeoutLayer` default.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(10_000).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms,
+ getter
+ )]
+ io_timeout_ms: NonZeroU64,
+}
+
+impl Default for OpenDalClientConfig {
+ fn default() -> Self {
+ Self {
+ io_timeout_ms: DEFAULT_IO_TIMEOUT_MS,
+ }
+ }
+}
+
+/// Parses one timeout value; the `Properties` derive adds the property-key context.
+/// Zero is rejected: it would time every operation out before it starts.
+fn parse_io_timeout_ms(value: &str) -> Result<NonZeroU64> {
+ value.parse().map_err(|_| {
Review Comment:
Done, using `.with_source(error)` like `parse_pool_property` in the SQL catalog. `"0"` now ends with `source: number would be zero for non-zero type`, and the parsing test checks the reason for every rejected value.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -221,10 +279,21 @@ fn default_memory_operator() -> Operator {
pub enum OpenDalStorage {
/// Memory storage variant.
#[cfg(feature = "opendal-memory")]
- Memory(#[serde(skip, default = "self::default_memory_operator")] Operator),
+ Memory {
+ /// Pre-built memory operator.
+ #[serde(skip, default = "self::default_memory_operator")]
+ operator: Operator,
+ /// Backend-independent client settings.
+ #[serde(default)]
+ client_config: OpenDalClientConfig,
+ },
/// Local filesystem storage variant.
#[cfg(feature = "opendal-fs")]
- LocalFs,
+ LocalFs {
Review Comment:
Confirmed for direct `OpenDalStorage` serialization, though the old `Memory` was the bare string `"Memory"`, not `{"Memory": null}`. `FileIO::serialize_all` is unaffected because it serializes the unchanged factory and properties, so I documented the break in the description and noted on `OpenDalStorage` that its serialized form is not stable across versions, as `FileIO::serialize_all` already says.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4153164908
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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:
small thing while we're here — the doc says `10000` but the const is `DEFAULT_IO_TIMEOUT_MS = 10_000`, so this can drift silently if the default ever changes. I'd write it as `defaults to 10_000 ms` and let the const stay the single source of truth.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Matches the `opendal::layers::TimeoutLayer` default.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(10_000).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms,
+ getter
+ )]
+ io_timeout_ms: NonZeroU64,
+}
+
+impl Default for OpenDalClientConfig {
+ fn default() -> Self {
+ Self {
+ io_timeout_ms: DEFAULT_IO_TIMEOUT_MS,
+ }
+ }
+}
+
+/// Parses one timeout value; the `Properties` derive adds the property-key context.
+/// Zero is rejected: it would time every operation out before it starts.
+fn parse_io_timeout_ms(value: &str) -> Result<NonZeroU64> {
+ value.parse().map_err(|_| {
Review Comment:
`map_err(|_| ...)` drops the underlying parse error, so `"abc"`, `"0"`, and `"18446744073709551616"` all surface the same message — we lose the std detail that says which one it was. I'd thread it through:
```rust
value.parse().map_err(|e| {
Error::new(ErrorKind::DataInvalid,
format!("Expected a positive integer number of milliseconds: {e}"))
.with_context("value", format!("{value:?}"))
})
```
##########
crates/storage/opendal/src/resolving.rs:
##########
@@ -377,6 +388,18 @@ mod tests {
}
}
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_resolve_propagates_io_timeout() {
Review Comment:
`from_properties` runs once before the scheme match, so propagation is scheme-independent — but this test only covers S3, and a build with just `opendal-memory` exercises none of it. I'd add a `memory://` (or `file://`) companion so a refactor that moves parsing into one match arm can't regress the others silently.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -221,10 +279,21 @@ fn default_memory_operator() -> Operator {
pub enum OpenDalStorage {
/// Memory storage variant.
#[cfg(feature = "opendal-memory")]
- Memory(#[serde(skip, default = "self::default_memory_operator")] Operator),
+ Memory {
+ /// Pre-built memory operator.
+ #[serde(skip, default = "self::default_memory_operator")]
+ operator: Operator,
+ /// Backend-independent client settings.
+ #[serde(default)]
+ client_config: OpenDalClientConfig,
+ },
/// Local filesystem storage variant.
#[cfg(feature = "opendal-fs")]
- LocalFs,
+ LocalFs {
Review Comment:
Making `LocalFs` and `Memory` struct variants is a serialized-format break, not just the API break the `!` marker covers. Externally-tagged serde wrote the old unit `LocalFs` as the bare string `"LocalFs"` and the old fully-skipped `Memory` newtype as `{"Memory": null}`; the new struct variants expect `{"LocalFs": {...}}` / `{"Memory": {...}}`. Any `OpenDalStorage` that was persisted to catalog properties, sent over RPC, or written to a config file will fail to deserialize at runtime after upgrade — and typetag makes this part of the `FileIO` serialization contract, so it's not hypothetical.
I'd either add back-compat deserialization that still accepts the old bare-string / `null` forms, or, if we're comfortable requiring a regen, call the wire-format break out explicitly in the description and release notes as distinct from the API break.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +476,46 @@ 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(
Review Comment:
The propagation tests confirm the value lands in `client_config`, but nothing exercises this line — that the timeout actually reaches `TimeoutLayer::with_io_timeout`. A refactor back to a bare `TimeoutLayer::new()` would pass every current test. It doesn't need a stall fixture; even asserting the wiring or a note on what's covered would close the gap.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,54 @@ 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`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Matches the `opendal::layers::TimeoutLayer` default.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(10_000).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms,
+ getter
+ )]
+ io_timeout_ms: NonZeroU64,
Review Comment:
`NonZeroU64: Copy`, so the generated getter handing back `&NonZeroU64` reads a bit odd — callers end up writing `*config.io_timeout_ms()`. Since this lands in `public-api.txt` on a crate that's about to publish, the reference shape is hard to walk back later.
I'd either add `NonZeroU64` to the macro's `is_copy_type` allowlist so it returns by value, or expose a `io_timeout(&self) -> Duration` accessor and keep the millisecond representation internal to the crate.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5780404944
@mbutrovich FYI
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4157597687
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +476,46 @@ 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(
Review Comment:
Added `test_io_timeout_reaches_timeout_layer`, which stalls every IO call with `ConcurrentLimitLayer::new(0)` under paused tokio time (`test-util`, dev only), so a read fails with `timeout: 45` from a 45000 ms setting. Reverting this line to a bare `TimeoutLayer::new()` makes it fail with `timeout: 10`.
##########
crates/storage/opendal/src/resolving.rs:
##########
@@ -377,6 +388,18 @@ mod tests {
}
}
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_resolve_propagates_io_timeout() {
Review Comment:
Done. The test is no longer S3-gated and checks every enabled scheme, so a memory-only build runs it, and handing the `memory` arm a default config makes it fail.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5981920890
Thanks for the pr! I’m excited to see more opendal integration. And thanks everyone for the reviews! 🚢
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4135238489
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +811,73 @@ 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", ""] {
Review Comment:
Could this also cover the edges of the valid range? `1` is the smallest accepted value, `u64::MAX` is the largest, and `18446744073709551616` (`u64::MAX + 1`) should be rejected. On the head commit, `u64::MAX` parses, and a write and read through `OpenDalStorage::Memory` configured with it both succeed, because tokio's `timeout` falls back to a far-future deadline when `Instant + Duration` overflows. `u64::MAX + 1` fails with `DataInvalid` and the property key in the context. A test would keep both behaviors from regressing. The suggestion is against the current `u64` field, so it would need `.get()` if the field becomes `NonZeroU64`.
```suggestion
assert_eq!(unset.io_timeout_ms(), DEFAULT_IO_TIMEOUT_MS);
assert_eq!(client_config("45000").unwrap().io_timeout_ms(), 45_000);
assert_eq!(client_config("1").unwrap().io_timeout_ms(), 1);
assert_eq!(client_config(&u64::MAX.to_string()).unwrap().io_timeout_ms(), u64::MAX);
for invalid in ["0", "-1", "12.5", "abc", "", "18446744073709551616"] {
```
##########
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`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Matches the `opendal::layers::TimeoutLayer` default.
+const DEFAULT_IO_TIMEOUT_MS: u64 = 10_000;
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms,
+ getter
+ )]
+ io_timeout_ms: u64,
+}
+
+impl Default for OpenDalClientConfig {
+ fn default() -> Self {
+ Self {
+ io_timeout_ms: DEFAULT_IO_TIMEOUT_MS,
+ }
+ }
+}
+
+/// Parses one timeout value; the `Properties` derive adds the property-key context.
+/// Zero is rejected: it would time every operation out before it starts.
+fn parse_io_timeout_ms(value: &str) -> Result<u64> {
+ match value.parse::<u64>() {
+ Ok(ms) if ms > 0 => Ok(ms),
+ _ => Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Expected a positive integer number of milliseconds",
+ )
+ .with_context("value", format!("{value:?}"))),
+ }
+}
Review Comment:
`parse_io_timeout_ms` rejects zero, but the derived `Deserialize` doesn't go through it. On the head commit, `serde_json::from_str::<OpenDalClientConfig>(r#"{"io_timeout_ms":0}"#)` succeeds with `io_timeout_ms() == 0`, and deserializing `{"LocalFs":{"client_config":{"io_timeout_ms":0}}}` as an `OpenDalStorage` does too. That's the path a serialized storage takes when it's rebuilt in another process, so a zero can reach `TimeoutLayer` without any error.
What do you think about making the field a `NonZeroU64`? `str::parse::<NonZeroU64>` rejects `"0"`, `"-1"`, `""`, and non-numeric input by itself, and serde's `NonZeroU64` impl rejects zero on deserialize, so both construction paths share one check. I tried the version below on the head commit. The existing tests pass once they call `.get()`, clippy is clean, and both zero payloads above fail with ``invalid value: integer `0`, expected a nonzero u64``. It also needs `use std::num::NonZeroU64;` and `.get()` at the `with_io_timeout` call in `create_operator`. The generated getter returns `&NonZeroU64`, because [`is_copy_type`](https://github.com/apache/iceberg-rust/blob/deab0692f6df211a396d6fc0ad21811f8b6c8491/crates/property-macro/src/properties.rs#L615-L662) doesn't list the `NonZero` types.
```suggestion
const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(10_000).unwrap();
/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
///
/// Fields are private, so later settings are additive rather than breaking. The
/// container-level serde default lets an older payload deserialize as new fields appear.
#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
#[serde(default)]
pub struct OpenDalClientConfig {
/// Per-attempt deadline for one IO operation, in milliseconds.
#[property(
key = OPENDAL_IO_TIMEOUT_MS,
default = DEFAULT_IO_TIMEOUT_MS,
parse_with = parse_io_timeout_ms,
getter
)]
io_timeout_ms: NonZeroU64,
}
impl Default for OpenDalClientConfig {
fn default() -> Self {
Self {
io_timeout_ms: DEFAULT_IO_TIMEOUT_MS,
}
}
}
/// Parses one timeout value; the `Properties` derive adds the property-key context.
/// Zero is rejected: it would time every operation out before it starts.
fn parse_io_timeout_ms(value: &str) -> Result<NonZeroU64> {
value.parse().map_err(|_| {
Error::new(
ErrorKind::DataInvalid,
"Expected a positive integer number of milliseconds",
)
.with_context("value", format!("{value:?}"))
})
}
```
If you'd rather keep `u64`, routing deserialization through the same validation with `#[serde(try_from = ...)]`, the pattern `FileScanTask` uses since #3091, would also work. Either way, could `test_client_config_serde_round_trip` add a case where a payload with a zero timeout fails to deserialize?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4169559300
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,65 @@ 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 [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`].
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
Review Comment:
Added the 60s, pinned to OpenDAL's default by `test_default_timeouts_match_opendal`, and the doc now names `exists` and `metadata` as the calls that keep it. Deletes and listing are not control operations in OpenDAL 0.58, which wraps deleters and listers in the IO timeout, and a stalled `delete` with a 45000 ms setting fails with `timeout: 45`.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,65 @@ 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 [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`].
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Default for [`OPENDAL_IO_TIMEOUT_MS`]. Matches the IO timeout default of OpenDAL's
+/// `TimeoutLayer`.
+pub const OPENDAL_IO_TIMEOUT_MS_DEFAULT: u64 = 10_000;
+
+/// [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`] as the field type. A zero default fails to compile.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(OPENDAL_IO_TIMEOUT_MS_DEFAULT).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms
+ )]
+ io_timeout_ms: NonZeroU64,
+}
+
+impl Default for OpenDalClientConfig {
+ fn default() -> Self {
+ // Reuse the `#[property]` defaults, so serde and `from_properties` cannot disagree.
+ Self::from_properties(&HashMap::new()).expect("every client setting has a default")
Review Comment:
Agreed, and the exposure is wider than absent payloads, since the container `#[serde(default)]` makes serde call `default()` on every `client_config` it deserializes. Reverted to the struct literal and added `test_default_matches_property_defaults`, which compares `default()` with `from_properties` on an empty map.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +825,122 @@ 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(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+
+ let max = u64::MAX.to_string();
+ for (valid, ms) in [("45000", 45_000), ("1", 1), (max.as_str(), u64::MAX)] {
+ assert_eq!(
+ client_config(valid).unwrap().io_timeout(),
+ Duration::from_millis(ms),
+ "{valid}"
+ );
+ }
+
+ for invalid in ["0", "-1", "12.5", "abc", "", "18446744073709551616"] {
+ let err = client_config(invalid).unwrap_err().to_string();
+ assert!(err.contains(OPENDAL_IO_TIMEOUT_MS), "{invalid}");
+ assert!(err.contains(&format!("value: {invalid:?}")), "{err}");
+ // The parse error is kept as the source, so the message says why it was rejected.
+ let reason = invalid.parse::<NonZeroU64>().unwrap_err().to_string();
+ assert!(err.contains(&reason), "{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.
+ let opendal_default = format!("{:?}", TimeoutLayer::new());
+ let with_io_timeout = |ms| {
+ format!(
+ "{:?}",
+ TimeoutLayer::new().with_io_timeout(Duration::from_millis(ms))
+ )
+ };
+ assert_eq!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ // If `Debug` stopped printing `io_timeout`, the check above would pass vacuously.
+ assert_ne!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT + 1)
+ );
+ }
+
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_client_config_serde_round_trip() {
Review Comment:
Added `test_old_unit_variant_forms_are_rejected` next to the round trip. It checks that `"LocalFs"`, `"Memory"` and `{"Memory": null}` all fail to deserialize.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +825,122 @@ 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(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+
+ let max = u64::MAX.to_string();
+ for (valid, ms) in [("45000", 45_000), ("1", 1), (max.as_str(), u64::MAX)] {
+ assert_eq!(
+ client_config(valid).unwrap().io_timeout(),
+ Duration::from_millis(ms),
+ "{valid}"
+ );
+ }
+
+ for invalid in ["0", "-1", "12.5", "abc", "", "18446744073709551616"] {
+ let err = client_config(invalid).unwrap_err().to_string();
+ assert!(err.contains(OPENDAL_IO_TIMEOUT_MS), "{invalid}");
+ assert!(err.contains(&format!("value: {invalid:?}")), "{err}");
+ // The parse error is kept as the source, so the message says why it was rejected.
+ let reason = invalid.parse::<NonZeroU64>().unwrap_err().to_string();
+ assert!(err.contains(&reason), "{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.
+ let opendal_default = format!("{:?}", TimeoutLayer::new());
+ let with_io_timeout = |ms| {
+ format!(
+ "{:?}",
+ TimeoutLayer::new().with_io_timeout(Duration::from_millis(ms))
+ )
+ };
+ assert_eq!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ // If `Debug` stopped printing `io_timeout`, the check above would pass vacuously.
+ assert_ne!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT + 1)
+ );
+ }
+
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_client_config_serde_round_trip() {
+ let storage = OpenDalStorage::S3 {
+ config: Arc::new(S3Config::default()),
+ customized_credential_load: None,
+ client_config: client_config("45000").unwrap(),
+ };
+
+ let mut value = serde_json::to_value(&storage).unwrap();
+ let restored: OpenDalStorage = serde_json::from_value(value.clone()).unwrap();
+ assert_eq!(
+ restored.client_config().io_timeout(),
+ Duration::from_secs(45)
+ );
+
+ // Deserializing rejects zero too, not only `from_properties`.
+ let mut zero = value.clone();
+ zero["S3"]["client_config"]["io_timeout_ms"] = 0.into();
+ assert!(serde_json::from_value::<OpenDalStorage>(zero).is_err());
+
+ // A payload without `client_config` falls back to the default.
+ value["S3"].as_object_mut().unwrap().remove("client_config");
+ let restored: OpenDalStorage = serde_json::from_value(value).unwrap();
+ assert_eq!(
+ restored.client_config().io_timeout(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ }
+
+ #[cfg(feature = "opendal-memory")]
+ #[test]
+ fn test_factory_rejects_invalid_io_timeout() {
+ let config = StorageConfig::new().with_prop(OPENDAL_IO_TIMEOUT_MS, "nope");
+
+ let err = OpenDalStorageFactory::Memory.build(&config).unwrap_err();
+ assert!(err.to_string().contains(OPENDAL_IO_TIMEOUT_MS));
+ }
+
+ #[cfg(feature = "opendal-memory")]
+ #[tokio::test(start_paused = true)]
+ async fn test_io_timeout_reaches_timeout_layer() {
+ use opendal::layers::ConcurrentLimitLayer;
+
+ // A concurrency limit of zero never grants a permit, so every IO call stalls until
+ // `TimeoutLayer` gives up. Paused time skips the timeouts and the retry backoff.
+ let storage = OpenDalStorage::Memory {
+ operator: default_memory_operator().layer(ConcurrentLimitLayer::new(0)),
+ client_config: client_config("45000").unwrap(),
+ };
+
+ let err = storage
+ .read("memory:/stalled")
+ .await
+ .unwrap_err()
+ .to_string();
+ assert!(err.contains("io operation timeout reached"), "{err}");
+ assert!(err.contains("timeout: 45"), "{err}");
Review Comment:
OpenDAL renders the budget as plain seconds (`timeout.as_secs_f64().to_string()`), so the error reads `context: { timeout: 45 }` and `"timeout: 45s"` would never match. The assertion now matches `{ timeout: 45 }`, whose closing brace rules out `450`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4074216850
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -27,3 +51,23 @@ pub(crate) fn from_opendal_error(e: opendal::Error) -> iceberg::Error {
)
.with_source(e)
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ fn props(value: &str) -> HashMap<String, String> {
+ HashMap::from([(CLIENT_IO_TIMEOUT_MS.to_string(), value.to_string())])
+ }
+
+ #[test]
+ fn test_io_timeout_ms_parse() {
+ assert_eq!(io_timeout_ms_parse(&HashMap::new()).unwrap(), 10_000);
Review Comment:
The tests duplicate the default value (`10_000`). Using `default_io_timeout_ms()` for the expected default would keep the test aligned if the default ever changes.
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -15,10 +15,34 @@
// specific language governing permissions and limitations
// under the License.
+use std::collections::HashMap;
+
+use iceberg::io::CLIENT_IO_TIMEOUT_MS;
+
pub(crate) fn is_truthy(value: &str) -> bool {
["true", "t", "1", "on"].contains(&value.to_lowercase().as_str())
}
+/// Matches the `opendal::layers::TimeoutLayer` default.
+pub(crate) fn default_io_timeout_ms() -> u64 {
+ 10_000
+}
+
+/// Parse iceberg props to the per-IO-operation timeout.
+pub(crate) fn io_timeout_ms_parse(m: &HashMap<String, String>) -> iceberg::Result<u64> {
+ let Some(value) = m.get(CLIENT_IO_TIMEOUT_MS) else {
+ return Ok(default_io_timeout_ms());
+ };
+ // Zero would make every operation time out before it starts.
+ match value.parse::<u64>() {
+ Ok(ms) if ms > 0 => Ok(ms),
+ _ => Err(iceberg::Error::new(
+ iceberg::ErrorKind::DataInvalid,
+ format!("Invalid {CLIENT_IO_TIMEOUT_MS}: {value}, expected a positive integer"),
+ )),
Review Comment:
For values like the empty string, the error renders as `Invalid client.io-timeout-ms: , ...`, which is ambiguous. Consider quoting/escaping the value (e.g., using debug formatting) so empty/whitespace inputs are clearly visible in the message.
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -15,10 +15,34 @@
// specific language governing permissions and limitations
// under the License.
+use std::collections::HashMap;
+
+use iceberg::io::CLIENT_IO_TIMEOUT_MS;
+
pub(crate) fn is_truthy(value: &str) -> bool {
["true", "t", "1", "on"].contains(&value.to_lowercase().as_str())
}
+/// Matches the `opendal::layers::TimeoutLayer` default.
+pub(crate) fn default_io_timeout_ms() -> u64 {
+ 10_000
+}
Review Comment:
Hardcoding `10_000` as the OpenDAL `TimeoutLayer` default is brittle: if OpenDAL changes its default, this comment and behavior will diverge. A more robust approach is to only call `with_io_timeout(...)` when `client.io-timeout-ms` is explicitly set, and otherwise rely on `TimeoutLayer::new()`'s default.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +429,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(),
Review Comment:
The wildcard arm (`_ => default_io_timeout_ms()`) plus `#[allow(unreachable_patterns)]` makes this match non-exhaustive and can silently fall back to the default if new `OpenDalStorage` variants are added later. Prefer making the match exhaustive (removing the `_` arm and the allow) so the compiler forces updates when a new backend/variant is introduced.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5935305009
@laskoviymishka The description now lists every variant that gained `client_config` and calls out the serialized-format break separately from the API break.
@xanderbailey Agreed. `FileIO::serialize_all` already says its form is not stable across crate versions, so I added the same note to `OpenDalStorage` instead of adding back-compat.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4088766448
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -163,35 +164,42 @@ where
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())?;
Review Comment:
Thanks both. Done in 18a0407. `OpenDalClientConfig` uses `#[derive(Properties)]`, and every `OpenDalStorage` variant carries it as `client`. Its field is private behind the generated getter, and a075964 adds a serde round-trip test for it.
One change from the sketch: the field is `io_timeout_ms: u64` rather than `io_timeout: Duration`. The derive's getter returns by value only for types it recognizes as `Copy`, which are mostly primitives, so a `Duration` field would get a getter returning `&Duration`. A `u64` also serializes as a plain integer in the key's own unit rather than as `{secs, nanos}`. Happy to switch to `Duration` if you prefer it.
##########
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:
Applied your wording in 18a0407. I also added a short paragraph saying the budget is per attempt, and that control operations such as `stat` and `rename` have a separate fixed budget.
##########
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:
Done in 18a0407. The method is now `OpenDalStorage::client()`, which returns `&OpenDalClientConfig`. Its `_` arm uses the same all-features-off gate as `create_operator`, with `opendal-memory` added, and the `allow` is gone. The enum has no variants in that build, so a075964 makes the arm `unreachable!()`.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -391,10 +429,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(),
Review Comment:
Fixed in 18a0407. The match now lives in `OpenDalStorage::client()`, which has no `#[allow(unreachable_patterns)]`. Its `_` arm only compiles when every backend feature is off, so a new variant without an arm is a compile error.
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -15,10 +15,34 @@
// specific language governing permissions and limitations
// under the License.
+use std::collections::HashMap;
+
+use iceberg::io::CLIENT_IO_TIMEOUT_MS;
+
pub(crate) fn is_truthy(value: &str) -> bool {
["true", "t", "1", "on"].contains(&value.to_lowercase().as_str())
}
+/// Matches the `opendal::layers::TimeoutLayer` default.
+pub(crate) fn default_io_timeout_ms() -> u64 {
+ 10_000
+}
Review Comment:
Kept a named `DEFAULT_IO_TIMEOUT_MS`, which is the shape suggested in review. OpenDAL's `TimeoutLayer` has no getter for its default, so a075964 adds `test_default_io_timeout_matches_opendal`. It compares the layer's `Debug` output and fails if an OpenDAL upgrade changes the default.
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -15,10 +15,34 @@
// specific language governing permissions and limitations
// under the License.
+use std::collections::HashMap;
+
+use iceberg::io::CLIENT_IO_TIMEOUT_MS;
+
pub(crate) fn is_truthy(value: &str) -> bool {
["true", "t", "1", "on"].contains(&value.to_lowercase().as_str())
}
+/// Matches the `opendal::layers::TimeoutLayer` default.
+pub(crate) fn default_io_timeout_ms() -> u64 {
+ 10_000
+}
+
+/// Parse iceberg props to the per-IO-operation timeout.
+pub(crate) fn io_timeout_ms_parse(m: &HashMap<String, String>) -> iceberg::Result<u64> {
+ let Some(value) = m.get(CLIENT_IO_TIMEOUT_MS) else {
+ return Ok(default_io_timeout_ms());
+ };
+ // Zero would make every operation time out before it starts.
+ match value.parse::<u64>() {
+ Ok(ms) if ms > 0 => Ok(ms),
+ _ => Err(iceberg::Error::new(
+ iceberg::ErrorKind::DataInvalid,
+ format!("Invalid {CLIENT_IO_TIMEOUT_MS}: {value}, expected a positive integer"),
+ )),
Review Comment:
Addressed in a075964. The rejected value now goes into the error context as `{value:?}`, so an empty input renders as `value: ""`. The error also names the property key.
##########
crates/storage/opendal/src/utils.rs:
##########
@@ -27,3 +51,23 @@ pub(crate) fn from_opendal_error(e: opendal::Error) -> iceberg::Error {
)
.with_source(e)
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ fn props(value: &str) -> HashMap<String, String> {
+ HashMap::from([(CLIENT_IO_TIMEOUT_MS.to_string(), value.to_string())])
+ }
+
+ #[test]
+ fn test_io_timeout_ms_parse() {
+ assert_eq!(io_timeout_ms_parse(&HashMap::new()).unwrap(), 10_000);
Review Comment:
Fixed in a075964. The test now asserts against `DEFAULT_IO_TIMEOUT_MS`, and `test_default_io_timeout_matches_opendal` pins that constant to OpenDAL's actual default.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4095163280
##########
crates/iceberg/src/io/storage/config/mod.rs:
##########
@@ -45,6 +45,14 @@ 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. Honored by every `iceberg-storage-opendal` backend, where
+/// it defaults to 10000 to match OpenDAL's `TimeoutLayer`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const CLIENT_IO_TIMEOUT_MS: &str = "client.io-timeout-ms";
Review Comment:
make sense, checking pattern
```
pub const iceberg::io::ADLS_SAS_TOKEN: &str
pub const iceberg::io::GCS_ALLOW_ANONYMOUS: &str
```
yeah, opendal would follow it and give more clarity
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4127675010
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -163,35 +214,42 @@ where
impl StorageFactory for OpenDalStorageFactory {
#[allow(unused_variables)]
fn build(&self, config: &StorageConfig) -> Result<Arc<dyn Storage>> {
+ let client = OpenDalClientConfig::from_properties(config.props())?;
Review Comment:
nit: can we call this client_config or opendal_config? rather than client? I think the name is a little confusing currently
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5960939518
@laskoviymishka Thanks. The title now drops the `datafusion` scope, and 189914c9c addresses the inline points.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5958249258
taking a look! sorry github ui is glitching for me
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5960148101
Thanks @laskoviymishka for the review, addressing
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4076360274
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -163,35 +164,42 @@ where
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())?;
Review Comment:
I'd go with a struct too, and build it with `#[derive(Properties)]`. @blackmwk asked for the derive on the new HDFS config in https://github.com/apache/iceberg-rust/pull/3111#discussion_r3892824029, and #3094 is moving the catalog configs over to it (see [`SqlCatalogProperties`](https://github.com/apache/iceberg-rust/blob/e694eea42032a3f0898ccee33f5d3dd0ca9cd24b/crates/catalog/sql/src/catalog.rs#L274-L311)). Something like this:
```rust
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal, datafusion)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4168878631
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,65 @@ 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 [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`].
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
Review Comment:
While we're documenting this, I'd give the "separate, fixed budget" line a concrete number — it's OpenDAL's 60s control-op default, and it also governs `delete` and the lister/deleter open, not just `stat`/`rename`. Otherwise someone who sets `io-timeout-ms=5000` to bound all their I/O gets surprised when metadata fetches and snapshot-expiry deletes still hang for up to a minute.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -100,6 +103,65 @@ 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 [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`].
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const OPENDAL_IO_TIMEOUT_MS: &str = "opendal.io-timeout-ms";
+
+/// Default for [`OPENDAL_IO_TIMEOUT_MS`]. Matches the IO timeout default of OpenDAL's
+/// `TimeoutLayer`.
+pub const OPENDAL_IO_TIMEOUT_MS_DEFAULT: u64 = 10_000;
+
+/// [`OPENDAL_IO_TIMEOUT_MS_DEFAULT`] as the field type. A zero default fails to compile.
+const DEFAULT_IO_TIMEOUT_MS: NonZeroU64 = NonZeroU64::new(OPENDAL_IO_TIMEOUT_MS_DEFAULT).unwrap();
+
+/// Backend-independent client settings, shared by every [`OpenDalStorage`] variant.
+///
+/// Fields are private, so later settings are additive rather than breaking. The
+/// container-level serde default lets an older payload deserialize as new fields appear.
+#[derive(Clone, Debug, Properties, Serialize, Deserialize)]
+#[serde(default)]
+pub struct OpenDalClientConfig {
+ /// Per-attempt deadline for one IO operation, in milliseconds.
+ #[property(
+ key = OPENDAL_IO_TIMEOUT_MS,
+ default = DEFAULT_IO_TIMEOUT_MS,
+ parse_with = parse_io_timeout_ms
+ )]
+ io_timeout_ms: NonZeroU64,
+}
+
+impl Default for OpenDalClientConfig {
+ fn default() -> Self {
+ // Reuse the `#[property]` defaults, so serde and `from_properties` cannot disagree.
+ Self::from_properties(&HashMap::new()).expect("every client setting has a default")
Review Comment:
I'd build this default directly (`Self { io_timeout_ms: DEFAULT_IO_TIMEOUT_MS }`) rather than route through `from_properties().expect()`. It's safe today, but the moment someone adds a `#[property]` field without a `default =`, this becomes a production panic on the deserialization path — serde calls `default()` for every absent `client_config`, so a stored payload missing that field would panic instead of erroring. If you want to keep the single-source guarantee, a `#[test]` asserting `default() == from_properties(&empty)` turns the drift into a CI failure instead.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +825,122 @@ 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(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+
+ let max = u64::MAX.to_string();
+ for (valid, ms) in [("45000", 45_000), ("1", 1), (max.as_str(), u64::MAX)] {
+ assert_eq!(
+ client_config(valid).unwrap().io_timeout(),
+ Duration::from_millis(ms),
+ "{valid}"
+ );
+ }
+
+ for invalid in ["0", "-1", "12.5", "abc", "", "18446744073709551616"] {
+ let err = client_config(invalid).unwrap_err().to_string();
+ assert!(err.contains(OPENDAL_IO_TIMEOUT_MS), "{invalid}");
+ assert!(err.contains(&format!("value: {invalid:?}")), "{err}");
+ // The parse error is kept as the source, so the message says why it was rejected.
+ let reason = invalid.parse::<NonZeroU64>().unwrap_err().to_string();
+ assert!(err.contains(&reason), "{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.
+ let opendal_default = format!("{:?}", TimeoutLayer::new());
+ let with_io_timeout = |ms| {
+ format!(
+ "{:?}",
+ TimeoutLayer::new().with_io_timeout(Duration::from_millis(ms))
+ )
+ };
+ assert_eq!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ // If `Debug` stopped printing `io_timeout`, the check above would pass vacuously.
+ assert_ne!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT + 1)
+ );
+ }
+
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_client_config_serde_round_trip() {
Review Comment:
The "Breaking (serialized form)" section covers the wire break well and I'm happy with the docs-only path we settled on last round — not reopening that. One cheap addition while you're in here: a test asserting an old bare-string `"LocalFs"` / `{"Memory": null}` payload now `is_err()` would pin the break as intended behavior rather than an accident, sitting right next to this round-trip test. Not blocking.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -687,6 +825,122 @@ 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(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+
+ let max = u64::MAX.to_string();
+ for (valid, ms) in [("45000", 45_000), ("1", 1), (max.as_str(), u64::MAX)] {
+ assert_eq!(
+ client_config(valid).unwrap().io_timeout(),
+ Duration::from_millis(ms),
+ "{valid}"
+ );
+ }
+
+ for invalid in ["0", "-1", "12.5", "abc", "", "18446744073709551616"] {
+ let err = client_config(invalid).unwrap_err().to_string();
+ assert!(err.contains(OPENDAL_IO_TIMEOUT_MS), "{invalid}");
+ assert!(err.contains(&format!("value: {invalid:?}")), "{err}");
+ // The parse error is kept as the source, so the message says why it was rejected.
+ let reason = invalid.parse::<NonZeroU64>().unwrap_err().to_string();
+ assert!(err.contains(&reason), "{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.
+ let opendal_default = format!("{:?}", TimeoutLayer::new());
+ let with_io_timeout = |ms| {
+ format!(
+ "{:?}",
+ TimeoutLayer::new().with_io_timeout(Duration::from_millis(ms))
+ )
+ };
+ assert_eq!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ // If `Debug` stopped printing `io_timeout`, the check above would pass vacuously.
+ assert_ne!(
+ opendal_default,
+ with_io_timeout(OPENDAL_IO_TIMEOUT_MS_DEFAULT + 1)
+ );
+ }
+
+ #[cfg(feature = "opendal-s3")]
+ #[test]
+ fn test_client_config_serde_round_trip() {
+ let storage = OpenDalStorage::S3 {
+ config: Arc::new(S3Config::default()),
+ customized_credential_load: None,
+ client_config: client_config("45000").unwrap(),
+ };
+
+ let mut value = serde_json::to_value(&storage).unwrap();
+ let restored: OpenDalStorage = serde_json::from_value(value.clone()).unwrap();
+ assert_eq!(
+ restored.client_config().io_timeout(),
+ Duration::from_secs(45)
+ );
+
+ // Deserializing rejects zero too, not only `from_properties`.
+ let mut zero = value.clone();
+ zero["S3"]["client_config"]["io_timeout_ms"] = 0.into();
+ assert!(serde_json::from_value::<OpenDalStorage>(zero).is_err());
+
+ // A payload without `client_config` falls back to the default.
+ value["S3"].as_object_mut().unwrap().remove("client_config");
+ let restored: OpenDalStorage = serde_json::from_value(value).unwrap();
+ assert_eq!(
+ restored.client_config().io_timeout(),
+ Duration::from_millis(OPENDAL_IO_TIMEOUT_MS_DEFAULT)
+ );
+ }
+
+ #[cfg(feature = "opendal-memory")]
+ #[test]
+ fn test_factory_rejects_invalid_io_timeout() {
+ let config = StorageConfig::new().with_prop(OPENDAL_IO_TIMEOUT_MS, "nope");
+
+ let err = OpenDalStorageFactory::Memory.build(&config).unwrap_err();
+ assert!(err.to_string().contains(OPENDAL_IO_TIMEOUT_MS));
+ }
+
+ #[cfg(feature = "opendal-memory")]
+ #[tokio::test(start_paused = true)]
+ async fn test_io_timeout_reaches_timeout_layer() {
+ use opendal::layers::ConcurrentLimitLayer;
+
+ // A concurrency limit of zero never grants a permit, so every IO call stalls until
+ // `TimeoutLayer` gives up. Paused time skips the timeouts and the retry backoff.
+ let storage = OpenDalStorage::Memory {
+ operator: default_memory_operator().layer(ConcurrentLimitLayer::new(0)),
+ client_config: client_config("45000").unwrap(),
+ };
+
+ let err = storage
+ .read("memory:/stalled")
+ .await
+ .unwrap_err()
+ .to_string();
+ assert!(err.contains("io operation timeout reached"), "{err}");
+ assert!(err.contains("timeout: 45"), "{err}");
Review Comment:
`"timeout: 45"` also matches `"timeout: 450"`, so this passes if the layer ever reports 450s — `"timeout: 45s"` with the unit pins it to the value you actually mean.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#issuecomment-5961012406
Will wait for some time before merging (looks like @kevinjqliu still looking)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(storage-opendal)!: make the per-IO-operation timeout configurable [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3263:
URL: https://github.com/apache/iceberg-rust/pull/3263#discussion_r4091052802
##########
crates/iceberg/src/io/storage/config/mod.rs:
##########
@@ -45,6 +45,14 @@ 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. Honored by every `iceberg-storage-opendal` backend, where
+/// it defaults to 10000 to match OpenDAL's `TimeoutLayer`.
+///
+/// Each retry attempt is bounded separately, so it is a per-attempt budget, not a total one.
+/// Control operations such as `stat` and `rename` are bounded by a separate, fixed budget.
+pub const CLIENT_IO_TIMEOUT_MS: &str = "client.io-timeout-ms";
Review Comment:
Given these are going to open dal specific configs, I wonder if we should namespace them as ‘opendal.’? And define them in the same place as the config itself?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org