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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
