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: [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]
