Repository navigation
fix: make the native Iceberg per-I/O timeout configurable - #6649
Conversation
Bump iceberg-rust to af1da4c (apache/iceberg-rust#3263), which adds the `opendal.io-timeout-ms` FileIO property, and set it from the new `spark.comet.iceberg.ioTimeout` config (default 30s) for native Iceberg scans and writes. Forward `opendal.*` keys to the native FileIO. The bump also needs two API updates: `FileWrite::close` now returns `FileMetadata`, and `UnboundPartitionField` is built with its builder. Closes apache#6124.
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Native Iceberg operations used OpenDAL’s fixed 10-second per-I/O timeout, which Comet could not configure.
- Design approach: Upgrade iceberg-rust and pass
spark.comet.iceberg.ioTimeoutthrough the existing scan/write property channel. - Correctness / compatibility analysis: No introduced P1/P2 issues found within this review. Reviewed all nine base-relative files and relevant upstream reader, storage, partition and writer changes. Compared duration parsing against Spark 3.4–4.2 sources and relevant behavior against supported Iceberg Java versions. Existing reviews, comments and threads were empty.
- Key design decisions: The positive-duration setting defaults to
30s. FileIO cache keys include the setting. The implementation reuses existing configuration and serialization infrastructure without adding per-row work. - Implementation sketch: Forward
opendal.io-timeout-ms, admit theopendal.property prefix, preserve returnedFileMetadata, and migrate partition-field construction to the upstream builder. - Behavioral changes worth calling out: Compared with
branch-1.1at992c806a7e38c2e88bd018aa5774164b0850e1fa, the per-I/O default intentionally increases from 10 to 30 seconds and becomes configurable. OpenDAL’s separate 60-second control-operation timeout remains unchanged. - Suggested improvements: No additional changes meeting the P1/P2 reporting threshold.
Reviewed full SHA 22abdf4151c6451bdbfa663e5be1f3166954e5c7 against 33f21da181c14df7fa3e6a3104c3d7215810147e. Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-iceberg-write-pr.
Validation: All 121 native Iceberg tests and the changed planner test passed. A disposable probe confirmed an actual OpenDAL read timeout at 25 ms and successful FileIO round trips with close metadata. The default-feature build encountered missing jni.h, so native tests ran with HDFS disabled.
Exact-head CI: 17 checks succeeded, two remained running, and 14 were skipped. Native build and Rust CI were pending. Spark SQL and Iceberg integration suites were skipped. Local JVM integration suites and real object-store validation were not run, so merge-readiness validation remains outstanding.
Which issue does this PR close?
Closes #6124.
Rationale for this change
Native Iceberg scans fail on OpenDAL's hard-wired 10s per-I/O timeout (
io operation timeout reached), and nothing in Comet could change it. apache/iceberg-rust#3263 made the timeout configurable through theopendal.io-timeout-msFileIO property.What changes are included in this PR?
icebergandiceberg-storage-opendalfrombb1e4a4toaf1da4c, the merge commit of feat(storage-opendal)!: make the per-IO-operation timeout configurable iceberg-rust#3263 (22 commits).spark.comet.iceberg.ioTimeout(default30s). The native Iceberg scan and write pass it to iceberg-rust asopendal.io-timeout-ms.opendal.to the nativeSTORAGE_PROPERTY_PREFIXESso the property reachesFileIO. The merged upstream key isopendal.io-timeout-ms, not theclient.io-timeout-msthe issue expected, so this is needed.FileWrite::closenow returnsFileMetadata, andUnboundPartitionFieldfields are private.How are these changes tested?
io_timeout_reaches_the_file_io: the key matches iceberg-rust'sOPENDAL_IO_TIMEOUT_MSand passes the storage-prefix filter.