Align storage backend with upstream's crates/storage/opendal - #80
Conversation
Phase 1 of migrating the fork onto upstream's crates/storage/opendal crate structure. Ports the ADLS anonymous-access opt-in (originally added in #79, on the old in-tree crates/iceberg/src/io/storage/opendal/azdls.rs) onto the new crate. opendal's Azdls service has no anonymous/unsigned mode of its own, unlike its S3 service's skip_signature. adls.allow-anonymous routes to opendal's generic Http service (unauthenticated GET+Range/HEAD) instead, scoped to the same account/container endpoint. Opt-in only, never inferred from "no credentials configured", since that can also legitimately mean "rely on ambient Azure credentials". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 2 of migrating the fork onto upstream's crates/storage/opendal crate structure. These were pub(crate), reachable only from inside crates/iceberg where RefreshableStorageFactory used to live. The replacement (a new, external crates/storage/refreshable crate, built only against public APIs) needs to read them from StorageConfig::props to recover table identity/metadata location for credential lookups. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 3 of migrating the fork onto upstream's crates/storage/opendal crate structure. Replaces the fork's old accessor-level credential- refresh wrapper (RefreshableAccessor/RefreshableStorage, ~1363 lines, implementing opendal's raw Service/oio::Read traits directly against the old in-tree OpenDalStorage) with a much simpler, call-level Storage wrapper built entirely against iceberg's public Storage/ StorageFactory traits and iceberg-storage-opendal's public API. RefreshableStorage wraps any injected Arc<dyn StorageFactory> (defaulting to OpenDalResolvingStorageFactory, which resolves the backend per call from the path's scheme, so no separate scheme tracking is needed at this layer). Every Storage method is retried at most once on any error: credentials are refreshed via the existing, upstream-shared StorageCredentialsLoader, the backend is rebuilt, and the call is retried against the fresh backend. Retrying on any error (rather than a narrower auth-specific heuristic) is exact behavioral parity with the old design, which had no error-kind filter either -- and is the only practical option anyway, since iceberg-storage-opendal's from_opendal_error flattens every opendal::Error to a single ErrorKind::Unexpected. delete_stream is not retried (its input Stream can't be safely replayed after partial consumption). Concurrency safety (double-checked locking via a credential version counter + an async refresh lock) is ported near-as-is from the old design. Unit-tested against a fake, backend-agnostic Storage/ StorageFactory pair -- no opendal or network needed -- including a concurrent-failures-trigger-exactly-one-refresh test, a coverage gap the old design's tests never had. Lives in its own workspace crate rather than inside crates/iceberg (where all of upstream's own churn concentrates) so this fork-only functionality never conflicts with future `git merge upstream/main` runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 4 of migrating the fork onto upstream's crates/storage/opendal
crate structure. Now that Phase 3 (iceberg-storage-refreshable) covers
every fork-only feature the old tree provided, delete it entirely:
- crates/iceberg/src/io/storage/opendal/ (the fork's pre-refactor
OpenDAL-based Storage/StorageFactory implementations for S3/GCS/OSS/
Azdls/fs/memory -- superseded by iceberg-storage-opendal).
- crates/iceberg/src/io/{refreshable_accessor,refreshable_storage}.rs
(superseded by crates/storage/refreshable).
crates/iceberg/Cargo.toml now matches upstream exactly: no opendal,
reqsign, reqsign-aws-v4, or reqsign-core dependencies, no storage-*
features. crates/iceberg/src/error.rs drops its
From<opendal::Error>/From<reqsign::Error> blanket conversions, no
longer needed now that nothing in this crate uses `?` on either.
A behavioral note: the old Azdls storage enforced that a path's scheme
(abfss/abfs/wasbs/wasb) exactly match what was configured at build
time; iceberg-storage-opendal has no equivalent check and accepts any
of those schemes uniformly. No on-record correctness/security reason
for the strict check was found, so this is intentionally not ported --
confirmed with the user.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 5 of migrating the fork onto upstream's crates/storage/opendal crate structure. Fixes up the one remaining in-workspace reference to the deleted old-tree RefreshableStorageFactory, and stale doc-comment cross-references left over from the same move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ction Audit against the deleted old tree found a real gap: the old RefreshableAccessor wrapped the *returned* opendal reader itself (RetryableReader), so every subsequent .read() call on a long-lived reader got credential-refresh-and-retry protection for the reader's whole lifetime. This crate's reader() only retried the call that constructed the Box<dyn FileRead> -- a reader obtained once and then used for many range reads (e.g. a Parquet scan, the primary scenario this whole mechanism exists to protect) got zero retry protection after the first successful construction. Fixed by having reader() return a RefreshableReader wrapper that resolves the current backend and constructs a fresh inner reader on every .read() call, retrying once on failure exactly like every other Storage method. This adds no real cost (Storage::reader() was already exactly this cheap per call) and no lock (range reads are naturally idempotent, so per-call re-resolution needs no shared mutable state, unlike write()). writer() is deliberately NOT given the same treatment and now has a doc comment explaining why: a writer is frequently stateful (e.g. a multipart upload), so replaying a failed write against a freshly-constructed writer could silently drop already-written bytes. This asymmetry matches the old design's own already-accepted lack of write retry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re-audited this migration against the deleted old tree (every backend's config/operator logic — S3/GCS/OSS/Azdls/fs/memory — plus the refresh mechanism) to make sure nothing fork-specific was silently dropped. Result: all backend config/operator-building logic is clean — no gaps. One real gap found and fixed (pushed as |
…overage
Continuing the audit against the deleted old tree, found two more gaps:
1. Behavioral: the old design's retry error message referenced both the
retry failure AND the original failure ("Original error: ..."), so a
confusing retry-side error (e.g. NotFound after a successful refresh)
didn't silently discard why a retry was attempted in the first place.
This crate's retry paths discarded the original error entirely. Fixed
via a shared refresh_and_retry() helper used by every Storage method
and by RefreshableReader::read, which attaches the original error as
`with_context("original_error", ...)` on the final error whenever the
retry itself also fails.
2. Test coverage: the old tree had tests this crate had no equivalent
for -- an end-to-end write-then-read-back roundtrip through a real
backend (test_refreshable_operator_can_write_and_read), and coverage
proving RefreshableStorageFactory::build() actually parses
PROP_METADATA_LOCATION/PROP_TABLE_IDENT out of props and threads them
to the credentials loader (test_refreshable_storage_factory_with_
initial_credentials_creates_refreshable only checked build() didn't
error, not that the values were used correctly). Added
write_and_read_roundtrip_through_real_backend (using the real
iceberg-storage-opendal Memory backend, not a fake) and
factory_build_parses_table_ident_and_location_from_props, plus
retry_failure_preserves_original_error_as_context for (1).
A few other old tests (test_build_from_props_memory,
test_build_from_props_s3_never_creates_refreshable,
test_routing_factory_routes_to_*, test_routing_factory_errors_on_missing_
location, test_do_refresh_rebuilds_inner_storage) were confirmed
structurally obsolete, not dropped coverage: they tested either the old
enum-variant-based OpenDalStorage::Refreshable design (replaced by a
standalone Storage impl) or OpenDalRoutingStorageFactory's build-time
scheme resolution (replaced by OpenDalResolvingStorage's per-call
resolution, which needs no location prop and so can't error on a
missing one).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Did a systematic test-coverage diff too (every `#[test]`/`#[tokio::test]` function name in the deleted old tree vs. the new one). Found two more real gaps in `iceberg-storage-refreshable`, both fixed (pushed as `d6a08e9`):
A handful of other old tests (`test_build_from_props_`, `test_routing_factory_`, `test_do_refresh_rebuilds_inner_storage`) were confirmed structurally obsolete rather than dropped coverage — they tested the old enum-variant/build-time-scheme-resolution design that no longer exists by construction, not a behavior that still needs proving. `iceberg-storage-refreshable` now has 9 unit tests (up from 6), all passing, plus clippy/fmt clean. |
Careful re-read of the old refreshable_accessor.rs (RetryableReader) surfaced a real performance regression in the read-retry fix from the previous commit: reconstructing a fresh inner reader via backend.reader(path) on *every* .read() call also rebuilds the whole opendal Operator from config every time (confirmed: every Storage method call does this). For a single call that's fine and matches every other method's cost profile, but reader() returns a long-lived handle meant to be read from many times (e.g. once per row group in a Parquet scan) -- rebuilding the Operator on every one of those reads multiplies that cost by the read count instead of paying it once, unlike the old design. The old RetryableReader avoided exactly this by holding one inner reader behind a tokio::sync::RwLock, read-locked (not serialized) on the happy path so concurrent range reads share it, and only write-locked briefly to swap in a freshly-rebuilt reader after a failure. RefreshableReader now does the same: reader() builds the initial inner reader once (with its own refresh-and-retry on construction failure), and .read() reuses it across calls, only refreshing and rebuilding on an actual failure. Added reader_reuses_inner_reader_across_successful_reads (a regression test using a FakeStorage that now counts reader() construction calls) to lock this in -- the existing reader_retries_individual_read_calls_not_just_construction test still passes unchanged, since the observable retry/refresh behavior is identical; only the internal reuse strategy changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Did another careful pass — this time a full line-by-line re-read of the old `mod.rs` (factory/enum dispatch, `create_operator`, `Storage` impl) and `refreshable_accessor.rs` (the raw opendal `Service` impl, `RetryableReader`), not just the test-name diff from before. Found and fixed one more real issue (pushed as `7f9abe1`): Performance regression: my `RefreshableReader::read()` was reconstructing the inner reader — which means rebuilding the entire `opendal::Operator` from config — on every single `.read()` call. That's fine for a one-shot operation, but a reader is typically obtained once and read from many times (e.g. once per row group in a Parquet scan), so this multiplied the rebuild cost by the read count instead of paying it once. The old `RetryableReader` avoided this by holding one inner reader behind a lock, shared (read-locked, not serialized) across concurrent reads on the happy path, and only rebuilding on an actual failure. `RefreshableReader` now does the same. Added a regression test (`reader_reuses_inner_reader_across_successful_reads`) that counts backend-rebuild calls to lock this in — the existing retry-behavior test is unaffected since the observable retry semantics didn't change, only the internal reuse strategy. Also re-confirmed, via direct reading (not just delegated audit): every backend's `create_operator`/`Storage` impl match arms account for all fork behavior (including the deliberate, already-approved `RetryLayer`-skip-for-Refreshable latency tradeoff), S3's `configured_scheme` concept is actually superseded by the new crate's per-call scheme echoing (more general, not a gap), and `create_dir`/`rename`/`presign`/`capability`/`info` (retried in the old opendal-`Service`-level design) have no equivalent in `iceberg`'s own `Storage` trait at all — never reachable above that abstraction boundary, so nothing to port there. `iceberg-storage-refreshable` is now at 10 unit tests, all green, clippy/fmt clean, full workspace check clean. |
1 participant
Summary
Migrates the fork off its old, pre-refactor in-tree OpenDAL storage implementation (
crates/iceberg/src/io/storage/opendal/) onto upstream's owncrates/storage/opendalcrate, which the fork already had as a workspace member (pulled in by the recent upstream merge) but never actually used. Net -2,767 lines.Goal: align as closely as possible with upstream so future
git merge upstream/mainruns don't keep re-touching this area — the old tree survived the last merge untouched (nothing to conflict with, since upstream deleted its own copy) and had silently drifted out of sync until a dependency bump broke it.What changed
adls.allow-anonymousported ontoiceberg-storage-opendal(this was added to the old tree very recently, in feat(io): support opt-in anonymous access for ADLS/Azure Blob storage #79 — same feature, just relocated to the new crate'sazdls.rs/lib.rs/resolving.rs).PROP_TABLE_IDENT/PROP_METADATA_LOCATIONincrates/icebergwidened frompub(crate)topub, needed by (4) below.crates/storage/refreshable(iceberg-storage-refreshable): replaces the old accessor-level credential-refresh wrapper (RefreshableAccessor/RefreshableStorage, ~1,363 lines, implementing opendal's rawService/oio::Readtraits directly against old-tree internals) with a much simpler ~600-line call-levelStoragewrapper built entirely againsticeberg's publicStorage/StorageFactorytraits andiceberg-storage-opendal's public API. EveryStoragemethod is retried once on any error: credentials refresh via the existing (already upstream-shared)StorageCredentialsLoader, the backend rebuilds, the call retries. Concurrency-safe (double-checked locking via a version counter + async refresh lock), unit-tested including a "concurrent failures trigger exactly one refresh" case.crates/iceberg/src/io/storage/opendal/and the old refresh files entirely.crates/iceberg/Cargo.tomlnow matches upstream exactly — zeroopendal/reqsign/reqsign-aws-v4/reqsign-coredependencies, nostorage-*features.crates/catalog/resttest) and stale doc comments.Preserved vs. intentionally dropped
CustomAwsCredentialLoader): the old bridge had zero in-repo callers andiceberg-storage-opendalalready ships its own native, cleaner equivalent (wrapsreqsign_core::ProvideCredentialdirectly) — not ported, just superseded.OpenDalRoutingStorageFactory): subsumed by upstream's ownOpenDalResolvingStorageFactory, now the default insideRefreshableStorageFactory.iceberg-storage-opendalhas no equivalent and accepts anyabfs*/wasb*scheme uniformly. No on-record correctness/security reason for the strict check was found; confirmed with the team before dropping it.Verification
All green:
cargo check --workspace --all-features --all-targets,make check-fmt check-clippy cargo-machete check-toml check-public-api,cargo test --workspace --all-features(unit + doc tests).🤖 Generated with Claude Code