From 38eae473556d1979026d2702c23a22185618878a Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Wed, 2 Sep 2026 16:05:01 +0200 Subject: [PATCH 1/8] feat(storage): port adls.allow-anonymous to iceberg-storage-opendal Phase 1 of migrating the fork onto upstream's crates/storage/opendal crate structure. Ports the ADLS anonymous-access opt-in (originally added in RelationalAI/iceberg-rust#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 --- crates/iceberg/public-api.txt | 6 +- crates/iceberg/src/io/storage/config/azdls.rs | 18 +++ crates/storage/opendal/Cargo.toml | 2 +- crates/storage/opendal/public-api.txt | 1 + crates/storage/opendal/src/azdls.rs | 116 +++++++++++++++++- crates/storage/opendal/src/lib.rs | 12 +- crates/storage/opendal/src/resolving.rs | 2 + 7 files changed, 146 insertions(+), 11 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 943659206a..8d3ce38b3a 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -732,6 +732,7 @@ pub mod iceberg::io::config pub struct iceberg::io::config::AzdlsConfig pub iceberg::io::config::AzdlsConfig::account_key: core::option::Option pub iceberg::io::config::AzdlsConfig::account_name: core::option::Option +pub iceberg::io::config::AzdlsConfig::allow_anonymous: bool pub iceberg::io::config::AzdlsConfig::authority_host: core::option::Option pub iceberg::io::config::AzdlsConfig::client_id: core::option::Option pub iceberg::io::config::AzdlsConfig::client_secret: core::option::Option @@ -754,7 +755,7 @@ impl core::fmt::Debug for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for iceberg::io::AzdlsConfig impl iceberg::io::AzdlsConfig -pub fn iceberg::io::AzdlsConfig::builder() -> AzdlsConfigBuilder<((), (), (), (), (), (), (), (), (), ())> +pub fn iceberg::io::AzdlsConfig::builder() -> AzdlsConfigBuilder<((), (), (), (), (), (), (), (), (), (), ())> impl serde_core::ser::Serialize for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::AzdlsConfig @@ -1108,6 +1109,7 @@ pub fn iceberg::io::opendal::OpenDalStorageFactory::deserialize<__D>(__deseriali pub struct iceberg::io::AzdlsConfig pub iceberg::io::AzdlsConfig::account_key: core::option::Option pub iceberg::io::AzdlsConfig::account_name: core::option::Option +pub iceberg::io::AzdlsConfig::allow_anonymous: bool pub iceberg::io::AzdlsConfig::authority_host: core::option::Option pub iceberg::io::AzdlsConfig::client_id: core::option::Option pub iceberg::io::AzdlsConfig::client_secret: core::option::Option @@ -1130,7 +1132,7 @@ impl core::fmt::Debug for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for iceberg::io::AzdlsConfig impl iceberg::io::AzdlsConfig -pub fn iceberg::io::AzdlsConfig::builder() -> AzdlsConfigBuilder<((), (), (), (), (), (), (), (), (), ())> +pub fn iceberg::io::AzdlsConfig::builder() -> AzdlsConfigBuilder<((), (), (), (), (), (), (), (), (), (), ())> impl serde_core::ser::Serialize for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::AzdlsConfig diff --git a/crates/iceberg/src/io/storage/config/azdls.rs b/crates/iceberg/src/io/storage/config/azdls.rs index f1d0c9fc79..b63c399a12 100644 --- a/crates/iceberg/src/io/storage/config/azdls.rs +++ b/crates/iceberg/src/io/storage/config/azdls.rs @@ -24,6 +24,7 @@ use typed_builder::TypedBuilder; use super::StorageConfig; use crate::Result; +use crate::io::is_truthy; /// A connection string. /// @@ -93,6 +94,9 @@ pub struct AzdlsConfig { /// Filesystem name. #[builder(default, setter(into))] pub filesystem: String, + /// Allow anonymous access. + #[builder(default)] + pub allow_anonymous: bool, } /// Finds the appropriate SAS token from properties based on account name. @@ -164,6 +168,11 @@ impl TryFrom<&StorageConfig> for AzdlsConfig { if let Some(authority_host) = props.get(ADLS_AUTHORITY_HOST) { cfg.authority_host = Some(authority_host.clone()); } + if let Some(allow_anonymous) = props.get(ADLS_ALLOW_ANONYMOUS) + && is_truthy(allow_anonymous.to_lowercase().as_str()) + { + cfg.allow_anonymous = true; + } Ok(cfg) } @@ -285,4 +294,13 @@ mod tests { Some("token-without-prefix") ); } + + #[test] + fn test_azdls_config_allow_anonymous() { + let storage_config = StorageConfig::new().with_prop(ADLS_ALLOW_ANONYMOUS, "true"); + + let azdls_config = AzdlsConfig::try_from(&storage_config).unwrap(); + + assert!(azdls_config.allow_anonymous); + } } diff --git a/crates/storage/opendal/Cargo.toml b/crates/storage/opendal/Cargo.toml index e43e7845b3..f000d821fe 100644 --- a/crates/storage/opendal/Cargo.toml +++ b/crates/storage/opendal/Cargo.toml @@ -39,7 +39,7 @@ opendal-all = [ "opendal-hf", ] -opendal-azdls = ["opendal/services-azdls"] +opendal-azdls = ["opendal/services-azdls", "opendal/services-http"] opendal-fs = ["opendal/services-fs"] opendal-gcs = ["opendal/services-gcs"] opendal-hf = ["opendal/services-hf"] diff --git a/crates/storage/opendal/public-api.txt b/crates/storage/opendal/public-api.txt index d8c4ecdb38..e76d6368c0 100644 --- a/crates/storage/opendal/public-api.txt +++ b/crates/storage/opendal/public-api.txt @@ -3,6 +3,7 @@ pub use iceberg_storage_opendal::AwsCredential pub use iceberg_storage_opendal::ProvideCredential pub enum iceberg_storage_opendal::OpenDalStorage pub iceberg_storage_opendal::OpenDalStorage::Azdls +pub iceberg_storage_opendal::OpenDalStorage::Azdls::allow_anonymous: bool pub iceberg_storage_opendal::OpenDalStorage::Azdls::config: alloc::sync::Arc pub iceberg_storage_opendal::OpenDalStorage::Gcs pub iceberg_storage_opendal::OpenDalStorage::Gcs::config: alloc::sync::Arc diff --git a/crates/storage/opendal/src/azdls.rs b/crates/storage/opendal/src/azdls.rs index 4e52d5ff1d..78148e1042 100644 --- a/crates/storage/opendal/src/azdls.rs +++ b/crates/storage/opendal/src/azdls.rs @@ -20,8 +20,8 @@ use std::fmt::Display; use std::str::FromStr; use iceberg::io::{ - ADLS_ACCOUNT_KEY, ADLS_ACCOUNT_NAME, ADLS_AUTHORITY_HOST, ADLS_CLIENT_ID, ADLS_CLIENT_SECRET, - ADLS_CONNECTION_STRING, ADLS_ENDPOINT, ADLS_SAS_TOKEN, ADLS_TENANT_ID, + ADLS_ACCOUNT_KEY, ADLS_ACCOUNT_NAME, ADLS_ALLOW_ANONYMOUS, ADLS_AUTHORITY_HOST, ADLS_CLIENT_ID, + ADLS_CLIENT_SECRET, ADLS_CONNECTION_STRING, ADLS_ENDPOINT, ADLS_SAS_TOKEN, ADLS_TENANT_ID, }; use iceberg::{Error, ErrorKind, Result}; use opendal::Configurator; @@ -29,7 +29,7 @@ use opendal::services::AzdlsConfig; use serde::{Deserialize, Serialize}; use url::Url; -use crate::utils::from_opendal_error; +use crate::utils::{from_opendal_error, is_truthy}; /// Local version of `ensure_data_valid` macro since the iceberg crate's macro /// uses `$crate::error::Error` paths that don't resolve from external crates @@ -117,6 +117,18 @@ pub(crate) fn azdls_config_parse(mut properties: HashMap) -> Res Ok(config) } +/// Returns whether `adls.allow-anonymous` is set on the given properties. +/// +/// This is read directly from the raw properties map, rather than through +/// [`AzdlsConfig`], since opendal's `AzdlsConfig` (unlike its S3 counterpart, +/// which has a `skip_signature` field) has no concept of anonymous access at +/// all -- see [`azdls_anonymous_operator_build`] for why. +pub(crate) fn azdls_allow_anonymous(properties: &HashMap) -> bool { + properties + .get(ADLS_ALLOW_ANONYMOUS) + .is_some_and(|v| is_truthy(v.to_lowercase().as_str())) +} + /// Builds an OpenDAL operator from the AzdlsConfig and path. /// /// The path is expected to include the scheme in a format like: @@ -124,11 +136,12 @@ pub(crate) fn azdls_config_parse(mut properties: HashMap) -> Res pub(crate) fn azdls_create_operator<'a>( absolute_path: &'a str, config: &AzdlsConfig, + allow_anonymous: bool, ) -> Result<(opendal::Operator, &'a str)> { let path = absolute_path.parse::()?; match_path_with_config(&path, config)?; - let op = azdls_config_build(config, &path)?; + let op = azdls_config_build(config, &path, allow_anonymous)?; // Paths to files in ADLS tend to be written in fully qualified form, // including their filesystem and account name. @@ -230,7 +243,41 @@ pub(crate) fn match_path_with_config(path: &AzureStoragePath, config: &AzdlsConf Ok(()) } -fn azdls_config_build(config: &AzdlsConfig, path: &AzureStoragePath) -> Result { +fn azdls_config_build( + config: &AzdlsConfig, + path: &AzureStoragePath, + allow_anonymous: bool, +) -> Result { + // opendal's Azdls service (via reqsign-azure-storage) has no anonymous/ + // unsigned mode: every request goes through a generic credential-provider + // chain whose `Credential` variants (SharedKey/SasToken/BearerToken) all + // require a non-empty secret to be considered valid, and the signer hard + // errors ("failed to load signing credential") if none can be found or + // validated -- unlike e.g. its S3 service, which exposes an explicit + // `skip_signature` bypass. So when no credential was configured at all, + // we can't get a genuinely anonymous request out of the Azdls service no + // matter what we pass it (an empty SAS token doesn't help either: it's + // treated as a *found but invalid* credential, which fails the same way). + // + // Public/anonymous-read Azure containers are plain, unauthenticated HTTPS + // (GET with a Range header for data, HEAD for stat) -- exactly what + // opendal's generic `Http` service already provides, with no built-in + // auth of its own unless a username/password/token is set. So route the + // opt-in anonymous case there instead, scoped to the same + // account/container endpoint the Azdls builder would have used. + // + // This is opt-in via `allow_anonymous` (see `ADLS_ALLOW_ANONYMOUS`), not + // inferred from "no credentials configured": the absence of an explicit + // adls.* credential can also legitimately mean "rely on ambient Azure + // credentials" (managed identity, Azure CLI, environment variables, all + // tried by the Azdls service's own default credential chain), which this + // must not silently break for callers who never asked for anonymous + // access. Whenever `allow_anonymous` is false, this always goes through + // the real, signed Azdls service exactly as before. + if allow_anonymous { + return azdls_anonymous_operator_build(config, path); + } + let mut builder = config.clone().into_builder(); if config.endpoint.is_none() { @@ -242,6 +289,27 @@ fn azdls_config_build(config: &AzdlsConfig, path: &AzureStoragePath) -> Result Result { + let endpoint = config + .endpoint + .clone() + .unwrap_or_else(|| path.as_endpoint()); + let mut http_config = opendal::services::HttpConfig::default(); + http_config.endpoint = Some(endpoint); + http_config.root = Some(format!("/{}", path.filesystem)); + // Deliberately leave username/password/token unset: opendal's Http + // service only adds an Authorization header when one of these is set, so + // leaving them unset is what makes this a genuinely unsigned request. + opendal::Operator::new(http_config.into_builder()).map_err(from_opendal_error) +} + /// Represents a fully qualified path to blob/ file in Azure Storage. #[derive(Debug, PartialEq)] pub(crate) struct AzureStoragePath { @@ -585,7 +653,7 @@ mod tests { ]; for (name, input, expected) in test_cases { - let result = azdls_create_operator(input.0, &input.1); + let result = azdls_create_operator(input.0, &input.1, false); match expected { Some((expected_filesystem, expected_path)) => { assert!(result.is_ok(), "Test case {name} failed: {result:?}"); @@ -601,6 +669,42 @@ mod tests { } } + #[test] + fn test_azdls_create_operator_anonymous() { + let path = "abfss://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet"; + let config = AzdlsConfig::default(); + + // No credentials, no `allow_anonymous`: goes through the real (signed) + // Azdls service as before -- unaffected by this fix. Building the + // operator itself still succeeds (loading a credential is deferred + // until an actual request is signed), so this only demonstrates it's + // *not* the anonymous HTTP fallback, via the scheme name. + let (op, relative_path) = azdls_create_operator(path, &config, false) + .expect("operator should build without a credential"); + assert_eq!(relative_path, "/path/to/file.parquet"); + assert_eq!(op.info().scheme(), "azdls"); + + // No credentials, `allow_anonymous=true`: routes to the anonymous + // HTTP fallback instead. + let (op, relative_path) = + azdls_create_operator(path, &config, true).expect("anonymous operator should build"); + assert_eq!(relative_path, "/path/to/file.parquet"); + assert_eq!(op.info().scheme(), "http"); + + // `allow_anonymous=true` takes priority even when a credential is + // also configured -- matching S3's `skip_signature`, which is checked + // unconditionally before any credential is loaded. Callers only ever + // set this alongside a real credential by mistake; since it's an + // explicit opt-in flag (never inferred), that mistake is on them. + let config_with_key = AzdlsConfig { + account_key: Some("secret".to_string()), + ..Default::default() + }; + let (op, _) = azdls_create_operator(path, &config_with_key, true) + .expect("anonymous operator should still build"); + assert_eq!(op.info().scheme(), "http"); + } + #[test] fn test_azure_storage_path_parse() { let test_cases = vec![ diff --git a/crates/storage/opendal/src/lib.rs b/crates/storage/opendal/src/lib.rs index 45da7aa23a..45899e5737 100644 --- a/crates/storage/opendal/src/lib.rs +++ b/crates/storage/opendal/src/lib.rs @@ -161,6 +161,7 @@ impl StorageFactory for OpenDalStorageFactory { })), #[cfg(feature = "opendal-azdls")] OpenDalStorageFactory::Azdls => Ok(Arc::new(OpenDalStorage::Azdls { + allow_anonymous: azdls_allow_anonymous(config.props()), config: azdls_config_parse(config.props().clone())?.into(), })), #[cfg(feature = "opendal-hf")] @@ -232,6 +233,9 @@ pub enum OpenDalStorage { Azdls { /// Azure DLS configuration. config: Arc, + /// Whether to allow genuinely anonymous, unsigned access when no + /// credential is configured. See `ADLS_ALLOW_ANONYMOUS`. + allow_anonymous: bool, }, /// HuggingFace Hub storage variant. /// @@ -335,7 +339,10 @@ impl OpenDalStorage { } } #[cfg(feature = "opendal-azdls")] - OpenDalStorage::Azdls { config } => azdls_create_operator(path, config)?, + OpenDalStorage::Azdls { + config, + allow_anonymous, + } => azdls_create_operator(path, config, *allow_anonymous)?, #[cfg(feature = "opendal-hf")] OpenDalStorage::Hf { config } => hf_config_build(config, path)?, #[cfg(all( @@ -453,7 +460,7 @@ impl OpenDalStorage { } } #[cfg(feature = "opendal-azdls")] - OpenDalStorage::Azdls { config } => { + OpenDalStorage::Azdls { config, .. } => { let azure_path = path.parse::()?; match_path_with_config(&azure_path, config)?; let relative_path_len = azure_path.path.len(); @@ -758,6 +765,7 @@ mod tests { endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()), ..Default::default() }), + allow_anonymous: false, }; assert_eq!( diff --git a/crates/storage/opendal/src/resolving.rs b/crates/storage/opendal/src/resolving.rs index 86993220a8..11aa60e2ef 100644 --- a/crates/storage/opendal/src/resolving.rs +++ b/crates/storage/opendal/src/resolving.rs @@ -111,9 +111,11 @@ fn build_storage_for_scheme( } #[cfg(feature = "opendal-azdls")] "azdls" => { + let allow_anonymous = crate::azdls::azdls_allow_anonymous(props); let config = crate::azdls::azdls_config_parse(props.clone())?; Ok(OpenDalStorage::Azdls { config: Arc::new(config), + allow_anonymous, }) } #[cfg(feature = "opendal-fs")] From a1e045e65850462cb88206456cdcd7c7d1de4406 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Wed, 2 Sep 2026 16:05:51 +0200 Subject: [PATCH 2/8] feat(io): expose PROP_TABLE_IDENT/PROP_METADATA_LOCATION publicly 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 --- crates/iceberg/public-api.txt | 4 ++++ crates/iceberg/src/io/storage/config/mod.rs | 11 +++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 8d3ce38b3a..0ba470d3a4 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -934,6 +934,8 @@ pub const iceberg::io::config::HF_TOKEN: &str pub const iceberg::io::config::OSS_ACCESS_KEY_ID: &str pub const iceberg::io::config::OSS_ACCESS_KEY_SECRET: &str pub const iceberg::io::config::OSS_ENDPOINT: &str +pub const iceberg::io::config::PROP_METADATA_LOCATION: &str +pub const iceberg::io::config::PROP_TABLE_IDENT: &str pub const iceberg::io::config::S3_ACCESS_KEY_ID: &str pub const iceberg::io::config::S3_ALLOW_ANONYMOUS: &str pub const iceberg::io::config::S3_ASSUME_ROLE_ARN: &str @@ -1488,6 +1490,8 @@ pub const iceberg::io::HF_TOKEN: &str pub const iceberg::io::OSS_ACCESS_KEY_ID: &str pub const iceberg::io::OSS_ACCESS_KEY_SECRET: &str pub const iceberg::io::OSS_ENDPOINT: &str +pub const iceberg::io::PROP_METADATA_LOCATION: &str +pub const iceberg::io::PROP_TABLE_IDENT: &str pub const iceberg::io::S3_ACCESS_KEY_ID: &str pub const iceberg::io::S3_ALLOW_ANONYMOUS: &str pub const iceberg::io::S3_ASSUME_ROLE_ARN: &str diff --git a/crates/iceberg/src/io/storage/config/mod.rs b/crates/iceberg/src/io/storage/config/mod.rs index 5ae34c6915..61e60a68e0 100644 --- a/crates/iceberg/src/io/storage/config/mod.rs +++ b/crates/iceberg/src/io/storage/config/mod.rs @@ -46,12 +46,15 @@ pub use s3::*; use serde::{Deserialize, Serialize}; /// Prop key under which the JSON-serialized `TableIdent` is stored in `StorageConfig::props`. -/// Consumed and stripped by `RefreshableStorageFactory::build()`; ignored by all other factories. -pub(crate) const PROP_TABLE_IDENT: &str = "iceberg.internal.table-ident"; +/// Consumed by third-party `StorageFactory` implementations that need table identity to look +/// up credentials (e.g. a refreshable/vended-credentials wrapper); ignored by the built-in +/// factories. +pub const PROP_TABLE_IDENT: &str = "iceberg.internal.table-ident"; /// Prop key under which the metadata location string is stored in `StorageConfig::props`. -/// Consumed and stripped by `RefreshableStorageFactory::build()`; ignored by all other factories. -pub(crate) const PROP_METADATA_LOCATION: &str = "iceberg.internal.metadata-location"; +/// Consumed by third-party `StorageFactory` implementations that need the table's metadata +/// location to look up credentials; ignored by the built-in factories. +pub const PROP_METADATA_LOCATION: &str = "iceberg.internal.metadata-location"; /// Configuration properties for storage backends. /// From dfcb06e399af50ff60af8173538270a9b11ee29f Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Wed, 2 Sep 2026 16:14:28 +0200 Subject: [PATCH 3/8] feat(storage): add iceberg-storage-refreshable crate 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 (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 --- Cargo.lock | 15 + Cargo.toml | 1 + crates/storage/refreshable/Cargo.toml | 45 ++ crates/storage/refreshable/public-api.txt | 34 ++ crates/storage/refreshable/src/lib.rs | 612 ++++++++++++++++++++++ 5 files changed, 707 insertions(+) create mode 100644 crates/storage/refreshable/Cargo.toml create mode 100644 crates/storage/refreshable/public-api.txt create mode 100644 crates/storage/refreshable/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index ebb17859d0..286da78ed6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4040,6 +4040,21 @@ dependencies = [ "url", ] +[[package]] +name = "iceberg-storage-refreshable" +version = "0.10.0" +dependencies = [ + "async-trait", + "bytes", + "futures", + "iceberg", + "iceberg-storage-opendal", + "serde", + "serde_json", + "tokio", + "typetag", +] + [[package]] name = "iceberg_test_utils" version = "0.10.0" diff --git a/Cargo.toml b/Cargo.toml index 38dd546f94..2c6e968035 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -110,6 +110,7 @@ iceberg-catalog-sql = { version = "0.10.0", path = "./crates/catalog/sql" } iceberg-datafusion = { version = "0.10.0", path = "./crates/integrations/datafusion" } iceberg-property-macro = { version = "0.10.0", path = "./crates/property-macro" } iceberg-storage-opendal = { version = "0.10.0", path = "./crates/storage/opendal" } +iceberg-storage-refreshable = { version = "0.10.0", path = "./crates/storage/refreshable" } indicatif = "0.18" itertools = "0.13" libtest-mimic = "0.8.1" diff --git a/crates/storage/refreshable/Cargo.toml b/crates/storage/refreshable/Cargo.toml new file mode 100644 index 0000000000..4bea5d4ecc --- /dev/null +++ b/crates/storage/refreshable/Cargo.toml @@ -0,0 +1,45 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +[package] +edition = { workspace = true } +license = { workspace = true } +name = "iceberg-storage-refreshable" +publish = false +repository = { workspace = true } +version = { workspace = true } + +categories = ["database"] +description = "Fork-only credential-refresh-on-error Storage wrapper for Apache Iceberg" +keywords = ["iceberg", "storage"] + +[dependencies] +async-trait = { workspace = true } +bytes = { workspace = true } +futures = { workspace = true } +iceberg = { workspace = true } +iceberg-storage-opendal = { workspace = true } +serde = { workspace = true } +serde_json = { workspace = true } +tokio = { workspace = true, features = ["sync"] } +typetag = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true, features = ["macros", "rt"] } + +[lints] +workspace = true diff --git a/crates/storage/refreshable/public-api.txt b/crates/storage/refreshable/public-api.txt new file mode 100644 index 0000000000..fafbcce8df --- /dev/null +++ b/crates/storage/refreshable/public-api.txt @@ -0,0 +1,34 @@ +pub mod iceberg_storage_refreshable +pub struct iceberg_storage_refreshable::RefreshableStorage +impl core::clone::Clone for iceberg_storage_refreshable::RefreshableStorage +pub fn iceberg_storage_refreshable::RefreshableStorage::clone(&self) -> Self +impl core::fmt::Debug for iceberg_storage_refreshable::RefreshableStorage +pub fn iceberg_storage_refreshable::RefreshableStorage::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg::io::storage::Storage for iceberg_storage_refreshable::RefreshableStorage +pub fn iceberg_storage_refreshable::RefreshableStorage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::delete_stream<'life0, 'async_trait>(&'life0 self, paths: futures_core::stream::BoxStream<'static, alloc::string::String>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::exists<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::metadata<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::new_input(&self, path: &str) -> iceberg::error::Result +pub fn iceberg_storage_refreshable::RefreshableStorage::new_output(&self, path: &str) -> iceberg::error::Result +pub fn iceberg_storage_refreshable::RefreshableStorage::read<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +pub fn iceberg_storage_refreshable::RefreshableStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait +impl serde_core::ser::Serialize for iceberg_storage_refreshable::RefreshableStorage +pub fn iceberg_storage_refreshable::RefreshableStorage::serialize(&self, serializer: S) -> core::result::Result<::Ok, ::Error> where S: serde_core::ser::Serializer +impl<'de> serde_core::de::Deserialize<'de> for iceberg_storage_refreshable::RefreshableStorage +pub fn iceberg_storage_refreshable::RefreshableStorage::deserialize(deserializer: D) -> core::result::Result::Error> where D: serde_core::de::Deserializer<'de> +pub struct iceberg_storage_refreshable::RefreshableStorageFactory +impl iceberg_storage_refreshable::RefreshableStorageFactory +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::new(credentials_loader: alloc::sync::Arc) -> Self +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::with_factory(self, factory: alloc::sync::Arc) -> Self +impl core::fmt::Debug for iceberg_storage_refreshable::RefreshableStorageFactory +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg::io::storage::StorageFactory for iceberg_storage_refreshable::RefreshableStorageFactory +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::build(&self, config: &iceberg::io::storage::config::StorageConfig) -> iceberg::error::Result> +impl serde_core::ser::Serialize for iceberg_storage_refreshable::RefreshableStorageFactory +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer +impl<'de> serde_core::de::Deserialize<'de> for iceberg_storage_refreshable::RefreshableStorageFactory +pub fn iceberg_storage_refreshable::RefreshableStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> diff --git a/crates/storage/refreshable/src/lib.rs b/crates/storage/refreshable/src/lib.rs new file mode 100644 index 0000000000..d059ec596d --- /dev/null +++ b/crates/storage/refreshable/src/lib.rs @@ -0,0 +1,612 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! A [`Storage`] wrapper that transparently refreshes credentials and retries once on +//! failure. +//! +//! This is fork-only functionality with no upstream equivalent: it exists to back the +//! REST catalog's vended-storage-credentials feature for tables whose credentials expire +//! while a `FileIO` built once, up front, is held and reused across many requests. It is +//! built entirely against `iceberg`'s public `Storage`/`StorageFactory` traits (see +//! `iceberg::io::storage::mod` for their contract) and `iceberg-storage-opendal`'s public +//! API — it never reaches into either crate's internals, so it stays out of the way of +//! future upstream syncs. +//! +//! Every [`Storage`] method call is retried at most once: on any error, credentials are +//! refreshed via the injected [`StorageCredentialsLoader`] and the whole backend is +//! rebuilt, then the original call is retried against the fresh backend. This is +//! deliberately coarse (call-level, not request-level) and deliberately triggers on any +//! error rather than trying to classify auth-shaped failures specifically — see the +//! [`RefreshableStorage`] docs for why. + +use std::collections::HashMap; +use std::fmt; +use std::sync::{Arc, RwLock}; + +use async_trait::async_trait; +use bytes::Bytes; +use futures::stream::BoxStream; +use iceberg::io::{ + FileMetadata, FileRead, FileWrite, InputFile, OutputFile, PROP_METADATA_LOCATION, + PROP_TABLE_IDENT, Storage, StorageConfig, StorageCredentialsLoader, StorageFactory, +}; +use iceberg::{Error, ErrorKind, NamespaceIdent, Result, TableIdent}; +use iceberg_storage_opendal::OpenDalResolvingStorageFactory; +use serde::{Deserialize, Serialize}; + +/// Shared, mutable state behind a [`RefreshableStorage`]. Lives behind a single `Arc` so +/// that cloning the outer handle (as `new_input`/`new_output` must, to hand a +/// `Storage`-implementing value to [`InputFile`]/[`OutputFile`]) shares state rather than +/// forking it — a refresh performed via one clone must be visible to every other clone +/// and to every already-issued `InputFile`/`OutputFile`. +struct State { + /// The current backend and the credential version it was built with, updated + /// together so a reader never observes a backend paired with the wrong version. + current: RwLock<(Arc, u64)>, + /// Factory used to (re)build the backend from merged base + credential properties. + /// Defaults to [`OpenDalResolvingStorageFactory`], which resolves the concrete + /// backend per call from the path's scheme rather than once at construction time — + /// so, unlike the old fork design, this needs no separate scheme tracking at all. + factory: Arc, + /// Base configuration properties (endpoint, region, etc.), excluding credentials and + /// the two internal props consumed by [`RefreshableStorageFactory::build`]. + base_props: HashMap, + credentials_loader: Arc, + location: String, + table_ident: TableIdent, + /// Serializes calls to `credentials_loader` so concurrent failures caused by the same + /// stale credentials trigger exactly one refresh, not a thundering herd. + refresh_lock: tokio::sync::Mutex<()>, +} + +impl State { + fn current(&self) -> (Arc, u64) { + let guard = self.current.read().expect("current lock poisoned"); + (guard.0.clone(), guard.1) + } + + /// Refreshes credentials and rebuilds the backend, unless another caller already did + /// so since `seen_version` was read (double-checked locking: cheap fast path with no + /// lock contention when a refresh already happened, `refresh_lock` serializes the + /// actual loader call when one is genuinely needed). + async fn refresh(&self, seen_version: u64) -> Result<()> { + if self.current.read().expect("current lock poisoned").1 > seen_version { + return Ok(()); + } + + let _guard = self.refresh_lock.lock().await; + + if self.current.read().expect("current lock poisoned").1 > seen_version { + return Ok(()); + } + + let new_creds = self + .credentials_loader + .load_credentials(&self.table_ident, &self.location) + .await?; + let mut props = self.base_props.clone(); + props.extend(new_creds.config); + let new_backend = self.factory.build(&StorageConfig::from_props(props))?; + + let mut guard = self.current.write().expect("current lock poisoned"); + let new_version = guard.1 + 1; + *guard = (new_backend, new_version); + Ok(()) + } +} + +/// A [`Storage`] that wraps another [`Storage`] (by default, any backend +/// [`iceberg-storage-opendal`](iceberg_storage_opendal) can build) and transparently +/// refreshes credentials on failure. +/// +/// # Retry behavior +/// +/// Every call is retried **at most once**: on any `Err`, credentials are refreshed and +/// the backend rebuilt, then the same call is retried against the fresh backend — whether +/// that retry succeeds or fails, its result is returned as-is (no further retries, no +/// backoff). This is deliberate, not a simplification of a more precise design: +/// +/// - The wrapped backend's errors don't reliably distinguish "credentials are stale" from +/// any other failure. `iceberg-storage-opendal` maps every `opendal::Error` to a single +/// flat `ErrorKind::Unexpected` regardless of cause, and a non-opendal `Storage` +/// implementation isn't obligated to preserve any particular error shape at all — so +/// there is no reliable signal to key a narrower heuristic on at this layer. +/// - Retrying on any error, unconditionally, is exact parity with this crate's +/// predecessor (an opendal-accessor-level wrapper), which had no error-kind filter +/// either. +/// +/// `delete_stream` is the one exception: the input `Stream` is consumed by the first +/// attempt and can't be safely replayed, so it is never retried — a failure always +/// propagates immediately, matching this crate's predecessor's already-accepted lack of +/// retry for several stream-based operations. +pub struct RefreshableStorage { + /// `None` only after deserialization, which cannot reconstruct live credential state + /// (the credentials loader and current backend are never serializable) — every method + /// returns an error in that case rather than panicking. + state: Option>, +} + +impl Clone for RefreshableStorage { + fn clone(&self) -> Self { + Self { + state: self.state.clone(), + } + } +} + +impl fmt::Debug for RefreshableStorage { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("RefreshableStorage").finish() + } +} + +// Manual, always-empty Serialize/Deserialize so this can satisfy `#[typetag::serde]`'s +// requirement on `Storage` despite genuinely holding no serializable state — mirrors how +// this crate's predecessor's `Refreshable` variant `#[serde(skip)]`-ed its backend field. +impl Serialize for RefreshableStorage { + fn serialize(&self, serializer: S) -> std::result::Result + where S: serde::Serializer { + serializer.serialize_unit() + } +} + +impl<'de> Deserialize<'de> for RefreshableStorage { + fn deserialize(deserializer: D) -> std::result::Result + where D: serde::Deserializer<'de> { + <()>::deserialize(deserializer)?; + Ok(Self { state: None }) + } +} + +impl RefreshableStorage { + fn new( + initial_backend: Arc, + factory: Arc, + base_props: HashMap, + credentials_loader: Arc, + location: String, + table_ident: TableIdent, + ) -> Self { + Self { + state: Some(Arc::new(State { + current: RwLock::new((initial_backend, 0)), + factory, + base_props, + credentials_loader, + location, + table_ident, + refresh_lock: tokio::sync::Mutex::new(()), + })), + } + } + + fn state(&self) -> Result<&Arc> { + self.state.as_ref().ok_or_else(|| { + Error::new( + ErrorKind::Unexpected, + "RefreshableStorage: deserialized instance has no live credential state and \ + cannot perform I/O", + ) + }) + } +} + +#[async_trait] +#[typetag::serde(name = "RefreshableStorage")] +impl Storage for RefreshableStorage { + async fn exists(&self, path: &str) -> Result { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.exists(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.exists(path).await + } + } + } + + async fn metadata(&self, path: &str) -> Result { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.metadata(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.metadata(path).await + } + } + } + + async fn read(&self, path: &str) -> Result { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.read(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.read(path).await + } + } + } + + async fn reader(&self, path: &str) -> Result> { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.reader(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.reader(path).await + } + } + } + + async fn write(&self, path: &str, bs: Bytes) -> Result<()> { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.write(path, bs.clone()).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.write(path, bs).await + } + } + } + + async fn writer(&self, path: &str) -> Result> { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.writer(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.writer(path).await + } + } + } + + async fn delete(&self, path: &str) -> Result<()> { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.delete(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.delete(path).await + } + } + } + + async fn delete_prefix(&self, path: &str) -> Result<()> { + let state = self.state()?; + let (backend, version) = state.current(); + match backend.delete_prefix(path).await { + Ok(v) => Ok(v), + Err(_) => { + state.refresh(version).await?; + state.current().0.delete_prefix(path).await + } + } + } + + /// Not retried — see the [`RefreshableStorage`] docs. + async fn delete_stream(&self, paths: BoxStream<'static, String>) -> Result<()> { + self.state()?.current().0.delete_stream(paths).await + } + + fn new_input(&self, path: &str) -> Result { + self.state()?; + Ok(InputFile::new(Arc::new(self.clone()), path.to_string())) + } + + fn new_output(&self, path: &str) -> Result { + self.state()?; + Ok(OutputFile::new(Arc::new(self.clone()), path.to_string())) + } +} + +/// A [`StorageFactory`] that builds a [`RefreshableStorage`]. +/// +/// Inject it at catalog construction time via `with_storage_factory`. At table-load time +/// the catalog populates [`StorageConfig`] with the table identity and metadata location +/// (`FileIOBuilder::with_table_ident`/`with_location`); `build()` reads those to give the +/// credentials loader context on every refresh. +/// +/// By default, credentials are refreshed against a backend built via +/// [`OpenDalResolvingStorageFactory`] (auto-detecting S3/GCS/OSS/ADLS/etc. from each +/// path), so one injected factory instance can serve tables across multiple storage +/// backends. Use [`RefreshableStorageFactory::with_factory`] to scope it to a specific, +/// pre-configured [`StorageFactory`] instead. +/// +/// # Example +/// +/// ```rust,no_run +/// use std::sync::Arc; +/// +/// use iceberg::io::StorageCredentialsLoader; +/// use iceberg_storage_refreshable::RefreshableStorageFactory; +/// +/// // Implement your own loader: +/// // let loader: Arc = ...; +/// // let factory = Arc::new(RefreshableStorageFactory::new(loader)); +/// // catalog_config.with_storage_factory(factory); +/// ``` +#[derive(Debug, Serialize, Deserialize)] +pub struct RefreshableStorageFactory { + /// `None` only after deserialization (field is skipped). + #[serde(skip)] + credentials_loader: Option>, + /// `None` means "use the default `OpenDalResolvingStorageFactory`", resolved lazily at + /// `build()` time rather than eagerly in `new()` so a plain `#[serde(skip)]` (with no + /// `Default` requirement on `Arc`) suffices here too. + #[serde(skip)] + inner_factory: Option>, +} + +impl RefreshableStorageFactory { + /// Creates a new factory that refreshes credentials via `credentials_loader`, building + /// backends via the default [`OpenDalResolvingStorageFactory`]. + pub fn new(credentials_loader: Arc) -> Self { + Self { + credentials_loader: Some(credentials_loader), + inner_factory: None, + } + } + + /// Scopes this factory to build backends via `factory` instead of the default + /// [`OpenDalResolvingStorageFactory`] — e.g. a pre-configured + /// `iceberg_storage_opendal::OpenDalStorageFactory::S3 { .. }` for a caller that only + /// ever serves one backend and wants to avoid per-call scheme resolution. + pub fn with_factory(mut self, factory: Arc) -> Self { + self.inner_factory = Some(factory); + self + } +} + +#[typetag::serde] +impl StorageFactory for RefreshableStorageFactory { + fn build(&self, config: &StorageConfig) -> Result> { + let loader = self.credentials_loader.as_ref().ok_or_else(|| { + Error::new( + ErrorKind::Unexpected, + "RefreshableStorageFactory: credentials loader unavailable after deserialization", + ) + })?; + + // Extract runtime context from props, stripping the internal keys so they don't + // leak into the wrapped backend's own configuration. + let mut props = config.props().clone(); + let location = props.remove(PROP_METADATA_LOCATION).unwrap_or_default(); + let table_ident = props + .remove(PROP_TABLE_IDENT) + .and_then(|s| serde_json::from_str::(&s).ok()) + .unwrap_or_else(|| { + TableIdent::new( + NamespaceIdent::new("unknown".to_string()), + "unknown".to_string(), + ) + }); + + let inner_factory = self + .inner_factory + .clone() + .unwrap_or_else(|| Arc::new(OpenDalResolvingStorageFactory::new())); + + let initial_backend = inner_factory.build(&StorageConfig::from_props(props.clone()))?; + + Ok(Arc::new(RefreshableStorage::new( + initial_backend, + inner_factory, + props, + Arc::clone(loader), + location, + table_ident, + ))) + } +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + + use iceberg::io::StorageCredential; + + use super::*; + + /// A minimal in-memory [`Storage`]/[`StorageFactory`] pair, independent of + /// `iceberg-storage-opendal`, so these tests stay fast and exercise only this crate's + /// own retry/refresh logic. `fail_until_version` lets a test simulate "this backend + /// works once a specific credential version is loaded". + #[derive(Debug, Clone, Serialize, Deserialize)] + struct FakeStorage { + version: u64, + #[serde(skip)] + fail_until_version: u64, + } + + #[async_trait] + #[typetag::serde(name = "FakeStorage")] + impl Storage for FakeStorage { + async fn exists(&self, _path: &str) -> Result { + if self.version < self.fail_until_version { + Err(Error::new(ErrorKind::Unexpected, "stale credentials")) + } else { + Ok(true) + } + } + async fn metadata(&self, _path: &str) -> Result { + unimplemented!() + } + async fn read(&self, _path: &str) -> Result { + unimplemented!() + } + async fn reader(&self, _path: &str) -> Result> { + unimplemented!() + } + async fn write(&self, _path: &str, _bs: Bytes) -> Result<()> { + unimplemented!() + } + async fn writer(&self, _path: &str) -> Result> { + unimplemented!() + } + async fn delete(&self, _path: &str) -> Result<()> { + unimplemented!() + } + async fn delete_prefix(&self, _path: &str) -> Result<()> { + unimplemented!() + } + async fn delete_stream(&self, _paths: BoxStream<'static, String>) -> Result<()> { + unimplemented!() + } + fn new_input(&self, _path: &str) -> Result { + unimplemented!() + } + fn new_output(&self, _path: &str) -> Result { + unimplemented!() + } + } + + #[derive(Debug, Clone, Serialize, Deserialize)] + struct FakeStorageFactory { + fail_until_version: u64, + } + + #[typetag::serde] + impl StorageFactory for FakeStorageFactory { + fn build(&self, config: &StorageConfig) -> Result> { + let version = config + .get("version") + .map(|v| v.parse().unwrap()) + .unwrap_or(0); + Ok(Arc::new(FakeStorage { + version, + fail_until_version: self.fail_until_version, + })) + } + } + + /// Returns credentials whose `config` bumps `version` by one each call, and tracks how + /// many times it was invoked. + struct TrackingLoader { + call_count: AtomicUsize, + } + + impl fmt::Debug for TrackingLoader { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("TrackingLoader").finish() + } + } + + #[async_trait] + impl StorageCredentialsLoader for TrackingLoader { + async fn load_credentials( + &self, + _table_ident: &TableIdent, + _location: &str, + ) -> Result { + let n = self.call_count.fetch_add(1, Ordering::SeqCst) + 1; + Ok(StorageCredential { + prefix: String::new(), + config: HashMap::from([("version".to_string(), n.to_string())]), + }) + } + } + + fn build(fail_until_version: u64, loader: Arc) -> Arc { + let factory = RefreshableStorageFactory::new(loader) + .with_factory(Arc::new(FakeStorageFactory { fail_until_version })); + factory.build(&StorageConfig::new()).unwrap() + } + + #[tokio::test] + async fn succeeds_first_try_without_refreshing() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let storage = build(0, Arc::clone(&loader)); + + assert!(storage.exists("x").await.unwrap()); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 0); + } + + #[tokio::test] + async fn refreshes_once_and_retries_on_failure() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + // Initial backend is built at version 0; the fake only succeeds once its own + // `version` is >= 1, i.e. only after exactly one refresh. + let storage = build(1, Arc::clone(&loader)); + + assert!(storage.exists("x").await.unwrap()); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + + // A second call against the now-fresh backend needs no further refresh. + assert!(storage.exists("x").await.unwrap()); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn propagates_error_when_retry_also_fails() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + // Never succeeds, no matter how many times it's refreshed. + let storage = build(u64::MAX, Arc::clone(&loader)); + + assert!(storage.exists("x").await.is_err()); + // Exactly one refresh attempt per call, not an infinite loop. + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn concurrent_failures_trigger_exactly_one_refresh() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let storage = build(1, Arc::clone(&loader)); + + let mut handles = Vec::new(); + for _ in 0..8 { + let storage = Arc::clone(&storage); + handles.push(tokio::spawn( + async move { storage.exists("x").await.unwrap() }, + )); + } + for h in handles { + assert!(h.await.unwrap()); + } + + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn refresh_is_visible_through_new_input_after_construction() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let storage = build(1, Arc::clone(&loader)); + + // new_input's returned InputFile must observe the *same* shared state as the + // storage it was created from -- not an independent snapshot -- so a refresh + // triggered through one still succeeds when read back through the other. + let input = storage.new_input("x").unwrap(); + assert!(input.exists().await.unwrap()); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + } +} From 9660425d2d5fd61e843fd03e53921070805b1ac9 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Wed, 2 Sep 2026 16:34:53 +0200 Subject: [PATCH 4/8] feat(storage): delete old in-tree opendal storage implementation 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/From 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 --- Cargo.lock | 44 +- crates/iceberg/Cargo.toml | 21 +- crates/iceberg/public-api.txt | 212 ---- crates/iceberg/src/error.rs | 5 - crates/iceberg/src/io/mod.rs | 2 - crates/iceberg/src/io/refreshable_accessor.rs | 914 ------------------ crates/iceberg/src/io/refreshable_storage.rs | 449 --------- crates/iceberg/src/io/storage/mod.rs | 6 - .../iceberg/src/io/storage/opendal/azdls.rs | 766 --------------- crates/iceberg/src/io/storage/opendal/fs.rs | 29 - crates/iceberg/src/io/storage/opendal/gcs.rs | 86 -- .../iceberg/src/io/storage/opendal/memory.rs | 25 - crates/iceberg/src/io/storage/opendal/mod.rs | 754 --------------- crates/iceberg/src/io/storage/opendal/oss.rs | 56 -- crates/iceberg/src/io/storage/opendal/s3.rs | 220 ----- crates/storage/refreshable/public-api.txt | 34 - 16 files changed, 2 insertions(+), 3621 deletions(-) delete mode 100644 crates/iceberg/src/io/refreshable_accessor.rs delete mode 100644 crates/iceberg/src/io/refreshable_storage.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/azdls.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/fs.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/gcs.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/memory.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/mod.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/oss.rs delete mode 100644 crates/iceberg/src/io/storage/opendal/s3.rs delete mode 100644 crates/storage/refreshable/public-api.txt diff --git a/Cargo.lock b/Cargo.lock index 286da78ed6..d7469cfc18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3763,15 +3763,11 @@ dependencies = [ "moka", "murmur3", "once_cell", - "opendal", "ordered-float 4.6.0", "parquet", "pretty_assertions", "rand 0.9.5", "regex", - "reqsign", - "reqsign-aws-v4", - "reqsign-core", "reqwest 0.12.28", "roaring", "serde", @@ -3869,6 +3865,7 @@ dependencies = [ "futures", "http 1.5.0", "iceberg", + "iceberg-storage-refreshable", "iceberg_test_utils", "itertools 0.13.0", "mockito", @@ -5942,16 +5939,6 @@ version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5a651516ddc9168ebd67b24afd085a718be02f8858fe406591b013d101ce2f40" -[[package]] -name = "quick-xml" -version = "0.37.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "331e97a1af0bf59823e6eadffe373d7b27f485be8748f71471c662c1f269b7fb" -dependencies = [ - "memchr", - "serde", -] - [[package]] name = "quick-xml" version = "0.39.4" @@ -6281,35 +6268,6 @@ version = "0.5.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "663ba70707f96e871406fe10d68128412e619b06d1d47cb91c3a4c6501176240" -[[package]] -name = "reqsign" -version = "0.16.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "43451dbf3590a7590684c25fb8d12ecdcc90ed3ac123433e500447c7d77ed701" -dependencies = [ - "anyhow", - "async-trait", - "base64 0.22.1", - "chrono", - "form_urlencoded", - "getrandom 0.2.17", - "hex", - "hmac 0.12.1", - "home", - "http 1.5.0", - "log", - "percent-encoding", - "quick-xml 0.37.5", - "rand 0.8.8", - "reqwest 0.12.28", - "rust-ini", - "serde", - "serde_json", - "sha1 0.10.7", - "sha2 0.10.9", - "tokio", -] - [[package]] name = "reqsign-aliyun-oss" version = "3.1.5" diff --git a/crates/iceberg/Cargo.toml b/crates/iceberg/Cargo.toml index e164f5ee39..56e11b1804 100644 --- a/crates/iceberg/Cargo.toml +++ b/crates/iceberg/Cargo.toml @@ -30,19 +30,7 @@ license = { workspace = true } repository = { workspace = true } [features] -default = ["storage-memory", "storage-fs", "storage-s3"] -storage-all = ["storage-memory", "storage-fs", "storage-s3", "storage-gcs"] -storage-azdls = ["opendal/services-azdls", "opendal/services-http"] -storage-fs = ["opendal/services-fs"] -storage-gcs = ["opendal/services-gcs"] -storage-memory = ["opendal/services-memory"] -storage-oss = ["opendal/services-oss"] -storage-s3 = [ - "opendal/services-s3", - "reqsign", - "dep:reqsign-aws-v4", - "dep:reqsign-core", -] +default = [] [dependencies] aes-gcm = { workspace = true } @@ -77,16 +65,9 @@ itertools = { workspace = true } moka = { version = "0.12.10", features = ["future"] } murmur3 = { workspace = true } once_cell = { workspace = true } -opendal = { workspace = true } ordered-float = { workspace = true } parquet = { workspace = true, features = ["async", "encryption"] } rand = { workspace = true } -reqsign = { version = "0.16.3", optional = true, default-features = false, features = [ - "reqwest_request", - "services-aws", -] } -reqsign-aws-v4 = { version = "3.0.0", optional = true } -reqsign-core = { version = "3.0.0", optional = true } reqwest = { workspace = true } roaring = { workspace = true } serde = { workspace = true } diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 0ba470d3a4..f508dc0a95 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -951,163 +951,6 @@ pub const iceberg::io::config::S3_SESSION_TOKEN: &str pub const iceberg::io::config::S3_SSE_KEY: &str pub const iceberg::io::config::S3_SSE_MD5: &str pub const iceberg::io::config::S3_SSE_TYPE: &str -pub mod iceberg::io::opendal -pub use iceberg::io::opendal::AwsCredential -pub use iceberg::io::opendal::AwsCredentialLoad -pub enum iceberg::io::opendal::OpenDalStorage -pub iceberg::io::opendal::OpenDalStorage::Azdls -pub iceberg::io::opendal::OpenDalStorage::Azdls::allow_anonymous: bool -pub iceberg::io::opendal::OpenDalStorage::Azdls::config: alloc::sync::Arc -pub iceberg::io::opendal::OpenDalStorage::Azdls::configured_scheme: iceberg::io::storage::opendal::azdls::AzureStorageScheme -pub iceberg::io::opendal::OpenDalStorage::Gcs -pub iceberg::io::opendal::OpenDalStorage::Gcs::config: alloc::sync::Arc -pub iceberg::io::opendal::OpenDalStorage::LocalFs -pub iceberg::io::opendal::OpenDalStorage::Memory(opendal_core::types::operator::operator::Operator) -pub iceberg::io::opendal::OpenDalStorage::Oss -pub iceberg::io::opendal::OpenDalStorage::Oss::config: alloc::sync::Arc -pub iceberg::io::opendal::OpenDalStorage::Refreshable -pub iceberg::io::opendal::OpenDalStorage::Refreshable::backend: core::option::Option> -pub iceberg::io::opendal::OpenDalStorage::S3 -pub iceberg::io::opendal::OpenDalStorage::S3::config: alloc::sync::Arc -pub iceberg::io::opendal::OpenDalStorage::S3::configured_scheme: alloc::string::String -pub iceberg::io::opendal::OpenDalStorage::S3::customized_credential_load: core::option::Option -impl core::clone::Clone for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::clone(&self) -> iceberg::io::opendal::OpenDalStorage -impl core::fmt::Debug for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::Storage for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_stream<'life0, 'async_trait>(&'life0 self, paths: futures_core::stream::BoxStream<'static, alloc::string::String>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::exists<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::metadata<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::new_input(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::new_output(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::read<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub enum iceberg::io::opendal::OpenDalStorageFactory -pub iceberg::io::opendal::OpenDalStorageFactory::Azdls -pub iceberg::io::opendal::OpenDalStorageFactory::Azdls::configured_scheme: iceberg::io::storage::opendal::azdls::AzureStorageScheme -pub iceberg::io::opendal::OpenDalStorageFactory::Fs -pub iceberg::io::opendal::OpenDalStorageFactory::Gcs -pub iceberg::io::opendal::OpenDalStorageFactory::Memory -pub iceberg::io::opendal::OpenDalStorageFactory::Oss -pub iceberg::io::opendal::OpenDalStorageFactory::S3 -pub iceberg::io::opendal::OpenDalStorageFactory::S3::configured_scheme: alloc::string::String -pub iceberg::io::opendal::OpenDalStorageFactory::S3::customized_credential_load: core::option::Option -impl core::clone::Clone for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::clone(&self) -> iceberg::io::opendal::OpenDalStorageFactory -impl core::fmt::Debug for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub struct iceberg::io::opendal::CustomAwsCredentialLoader(_) -pub struct iceberg::io::opendal::CustomAwsCredentialLoader(_) -impl iceberg::io::opendal::CustomAwsCredentialLoader -impl iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::into_opendal_loader(self) -> alloc::boxed::Box -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::into_opendal_loader(self) -> alloc::boxed::Box -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::new(loader: alloc::sync::Arc) -> Self -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::new(loader: alloc::sync::Arc) -> Self -impl core::clone::Clone for iceberg::io::opendal::CustomAwsCredentialLoader -impl core::clone::Clone for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::clone(&self) -> iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::clone(&self) -> iceberg::io::opendal::CustomAwsCredentialLoader -impl core::fmt::Debug for iceberg::io::opendal::CustomAwsCredentialLoader -impl core::fmt::Debug for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl reqsign::aws::credential::CredentialLoad for iceberg::io::opendal::CustomAwsCredentialLoader -impl reqsign::aws::credential::CredentialLoad for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::load_credential<'life0, 'async_trait>(&'life0 self, client: reqwest::async_impl::client::Client) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::load_credential<'life0, 'async_trait>(&'life0 self, client: reqwest::async_impl::client::Client) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub struct iceberg::io::opendal::OpenDalRoutingStorageFactory -impl core::fmt::Debug for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub struct iceberg::io::opendal::RefreshableStorageFactory -impl iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::new(credentials_loader: alloc::sync::Arc) -> Self -impl core::fmt::Debug for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub enum iceberg::io::OpenDalStorage -pub iceberg::io::OpenDalStorage::Azdls -pub iceberg::io::OpenDalStorage::Azdls::allow_anonymous: bool -pub iceberg::io::OpenDalStorage::Azdls::config: alloc::sync::Arc -pub iceberg::io::OpenDalStorage::Azdls::configured_scheme: iceberg::io::storage::opendal::azdls::AzureStorageScheme -pub iceberg::io::OpenDalStorage::Gcs -pub iceberg::io::OpenDalStorage::Gcs::config: alloc::sync::Arc -pub iceberg::io::OpenDalStorage::LocalFs -pub iceberg::io::OpenDalStorage::Memory(opendal_core::types::operator::operator::Operator) -pub iceberg::io::OpenDalStorage::Oss -pub iceberg::io::OpenDalStorage::Oss::config: alloc::sync::Arc -pub iceberg::io::OpenDalStorage::Refreshable -pub iceberg::io::OpenDalStorage::Refreshable::backend: core::option::Option> -pub iceberg::io::OpenDalStorage::S3 -pub iceberg::io::OpenDalStorage::S3::config: alloc::sync::Arc -pub iceberg::io::OpenDalStorage::S3::configured_scheme: alloc::string::String -pub iceberg::io::OpenDalStorage::S3::customized_credential_load: core::option::Option -impl core::clone::Clone for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::clone(&self) -> iceberg::io::opendal::OpenDalStorage -impl core::fmt::Debug for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::Storage for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_stream<'life0, 'async_trait>(&'life0 self, paths: futures_core::stream::BoxStream<'static, alloc::string::String>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::exists<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::metadata<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::new_input(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::new_output(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::read<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub enum iceberg::io::OpenDalStorageFactory -pub iceberg::io::OpenDalStorageFactory::Azdls -pub iceberg::io::OpenDalStorageFactory::Azdls::configured_scheme: iceberg::io::storage::opendal::azdls::AzureStorageScheme -pub iceberg::io::OpenDalStorageFactory::Fs -pub iceberg::io::OpenDalStorageFactory::Gcs -pub iceberg::io::OpenDalStorageFactory::Memory -pub iceberg::io::OpenDalStorageFactory::Oss -pub iceberg::io::OpenDalStorageFactory::S3 -pub iceberg::io::OpenDalStorageFactory::S3::configured_scheme: alloc::string::String -pub iceberg::io::OpenDalStorageFactory::S3::customized_credential_load: core::option::Option -impl core::clone::Clone for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::clone(&self) -> iceberg::io::opendal::OpenDalStorageFactory -impl core::fmt::Debug for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> pub struct iceberg::io::AzdlsConfig pub iceberg::io::AzdlsConfig::account_key: core::option::Option pub iceberg::io::AzdlsConfig::account_name: core::option::Option @@ -1139,16 +982,6 @@ impl serde_core::ser::Serialize for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::AzdlsConfig pub fn iceberg::io::AzdlsConfig::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> -pub struct iceberg::io::CustomAwsCredentialLoader(_) -impl iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::into_opendal_loader(self) -> alloc::boxed::Box -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::new(loader: alloc::sync::Arc) -> Self -impl core::clone::Clone for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::clone(&self) -> iceberg::io::opendal::CustomAwsCredentialLoader -impl core::fmt::Debug for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl reqsign::aws::credential::CredentialLoad for iceberg::io::opendal::CustomAwsCredentialLoader -pub fn iceberg::io::opendal::CustomAwsCredentialLoader::load_credential<'life0, 'async_trait>(&'life0 self, client: reqwest::async_impl::client::Client) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg::io::FileIO impl iceberg::io::FileIO pub fn iceberg::io::FileIO::config(&self) -> &iceberg::io::StorageConfig @@ -1321,15 +1154,6 @@ impl core::clone::Clone for iceberg::io::MetadataLocation pub fn iceberg::io::MetadataLocation::clone(&self) -> iceberg::io::MetadataLocation impl core::fmt::Debug for iceberg::io::MetadataLocation pub fn iceberg::io::MetadataLocation::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -pub struct iceberg::io::OpenDalRoutingStorageFactory -impl core::fmt::Debug for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> pub struct iceberg::io::OssConfig pub iceberg::io::OssConfig::access_key_id: core::option::Option pub iceberg::io::OssConfig::access_key_secret: core::option::Option @@ -1364,17 +1188,6 @@ pub async fn iceberg::io::OutputFile::write(&self, bs: bytes::bytes::Bytes) -> i pub async fn iceberg::io::OutputFile::writer(&self) -> iceberg::Result> impl core::fmt::Debug for iceberg::io::OutputFile pub fn iceberg::io::OutputFile::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -pub struct iceberg::io::RefreshableStorageFactory -impl iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::new(credentials_loader: alloc::sync::Arc) -> Self -impl core::fmt::Debug for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::StorageFactory for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl serde_core::ser::Serialize for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> pub struct iceberg::io::S3Config pub iceberg::io::S3Config::access_key_id: core::option::Option pub iceberg::io::S3Config::allow_anonymous: bool @@ -1511,8 +1324,6 @@ pub trait iceberg::io::FileRead: core::marker::Send + core::marker::Sync + core: pub fn iceberg::io::FileRead::read<'life0, 'async_trait>(&'life0 self, range: core::ops::range::Range) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait impl iceberg::io::FileRead for iceberg::encryption::AesGcmFileRead pub fn iceberg::encryption::AesGcmFileRead::read<'life0, 'async_trait>(&'life0 self, range: core::ops::range::Range) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -impl iceberg::io::FileRead for opendal_core::types::read::reader::Reader -pub fn opendal_core::types::read::reader::Reader::read<'life0, 'async_trait>(&'life0 self, range: core::ops::range::Range) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait impl + core::marker::Send + core::marker::Sync + core::marker::Unpin + 'static> iceberg::io::FileRead for T pub fn T::read<'life0, 'async_trait>(&'life0 self, range: core::ops::range::Range) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub trait iceberg::io::FileWrite: core::marker::Send + core::marker::Unpin + 'static @@ -1521,9 +1332,6 @@ pub fn iceberg::io::FileWrite::write<'life0, 'async_trait>(&'life0 mut self, bs: impl iceberg::io::FileWrite for iceberg::encryption::AesGcmFileWrite pub fn iceberg::encryption::AesGcmFileWrite::close<'life0, 'async_trait>(&'life0 mut self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub fn iceberg::encryption::AesGcmFileWrite::write<'life0, 'async_trait>(&'life0 mut self, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -impl iceberg::io::FileWrite for opendal_core::types::write::writer::Writer -pub fn opendal_core::types::write::writer::Writer::close<'life0, 'async_trait>(&'life0 mut self) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn opendal_core::types::write::writer::Writer::write<'life0, 'async_trait>(&'life0 mut self, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub trait iceberg::io::Storage: core::fmt::Debug + core::marker::Send + core::marker::Sync + typetag::Serialize + typetag::Deserialize pub fn iceberg::io::Storage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait pub fn iceberg::io::Storage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait @@ -1560,18 +1368,6 @@ pub fn iceberg::io::MemoryStorage::read<'life0, 'life1, 'async_trait>(&'life0 se pub fn iceberg::io::MemoryStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait pub fn iceberg::io::MemoryStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait pub fn iceberg::io::MemoryStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -impl iceberg::io::Storage for iceberg::io::opendal::OpenDalStorage -pub fn iceberg::io::opendal::OpenDalStorage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::delete_stream<'life0, 'async_trait>(&'life0 self, paths: futures_core::stream::BoxStream<'static, alloc::string::String>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::exists<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::metadata<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::new_input(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::new_output(&self, path: &str) -> iceberg::Result -pub fn iceberg::io::opendal::OpenDalStorage::read<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg::io::opendal::OpenDalStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait pub trait iceberg::io::StorageCredentialsLoader: core::marker::Send + core::marker::Sync + core::fmt::Debug pub fn iceberg::io::StorageCredentialsLoader::load_credentials<'life0, 'life1, 'life2, 'async_trait>(&'life0 self, table_ident: &'life1 iceberg::TableIdent, location: &'life2 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait, 'life2: 'async_trait pub trait iceberg::io::StorageFactory: core::fmt::Debug + core::marker::Send + core::marker::Sync + typetag::Serialize + typetag::Deserialize @@ -1580,12 +1376,6 @@ impl iceberg::io::StorageFactory for iceberg::io::LocalFsStorageFactory pub fn iceberg::io::LocalFsStorageFactory::build(&self, _config: &iceberg::io::StorageConfig) -> iceberg::Result> impl iceberg::io::StorageFactory for iceberg::io::MemoryStorageFactory pub fn iceberg::io::MemoryStorageFactory::build(&self, _config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalRoutingStorageFactory -pub fn iceberg::io::opendal::OpenDalRoutingStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl iceberg::io::StorageFactory for iceberg::io::opendal::OpenDalStorageFactory -pub fn iceberg::io::opendal::OpenDalStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> -impl iceberg::io::StorageFactory for iceberg::io::opendal::RefreshableStorageFactory -pub fn iceberg::io::opendal::RefreshableStorageFactory::build(&self, config: &iceberg::io::StorageConfig) -> iceberg::Result> pub mod iceberg::memory pub struct iceberg::memory::MemoryCatalog impl core::fmt::Debug for iceberg::memory::MemoryCatalog @@ -4284,8 +4074,6 @@ impl core::convert::From for iceberg::Error pub fn iceberg::Error::from(v: core::str::error::Utf8Error) -> Self impl core::convert::From for iceberg::Error pub fn iceberg::Error::from(v: futures_channel::mpsc::SendError) -> Self -impl core::convert::From for iceberg::Error -pub fn iceberg::Error::from(v: opendal_core::types::error::Error) -> Self impl core::convert::From for iceberg::Error pub fn iceberg::Error::from(v: parquet::errors::ParquetError) -> Self impl core::convert::From for iceberg::Error diff --git a/crates/iceberg/src/error.rs b/crates/iceberg/src/error.rs index ad91473612..06a4f0232a 100644 --- a/crates/iceberg/src/error.rs +++ b/crates/iceberg/src/error.rs @@ -442,11 +442,6 @@ define_from_err!( ); define_from_err!(std::io::Error, ErrorKind::Unexpected, "IO Operation failed"); -define_from_err!( - opendal::Error, - ErrorKind::Unexpected, - "Failure in doing io operation" -); /// Converts a [`PoisonError`] from a poisoned lock into an [`Error`]. pub(crate) fn lock_error(e: PoisonError) -> Error { diff --git a/crates/iceberg/src/io/mod.rs b/crates/iceberg/src/io/mod.rs index 9967d075e1..53586ea9f4 100644 --- a/crates/iceberg/src/io/mod.rs +++ b/crates/iceberg/src/io/mod.rs @@ -50,8 +50,6 @@ //! - `new_output`: Create output file for writing. mod file_io; -mod refreshable_accessor; -mod refreshable_storage; mod storage; mod storage_credential; diff --git a/crates/iceberg/src/io/refreshable_accessor.rs b/crates/iceberg/src/io/refreshable_accessor.rs deleted file mode 100644 index 86ae592a2d..0000000000 --- a/crates/iceberg/src/io/refreshable_accessor.rs +++ /dev/null @@ -1,914 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::future::Future; -use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::{Arc, Mutex}; - -use opendal::raw::oio::Read as OioRead; -use opendal::raw::*; -use opendal::{Buffer, BytesRange, Capability, OperationContext}; -use tokio::sync::RwLock as AsyncRwLock; - -use super::refreshable_storage::RefreshableOpenDalStorage; -use crate::Result; - -/// An OpenDAL accessor that wraps another accessor and retries after refreshing -/// credentials when any operation fails. -/// -/// Each instance has its own inner accessor and shares credential state with -/// other accessors via `Arc`. Credentials are only -/// refreshed when an operation fails, not proactively. -/// -/// Concurrency: if multiple accessors hit errors simultaneously, only one will -/// call the external credential loader (via double-checked locking on -/// `RefreshableOpenDalStorage::try_refresh_credentials`). The others will detect -/// the version bump and simply rebuild their accessor from the already-refreshed -/// credentials. -/// -/// # Known limitation (opendal 0.58 `Service` trait) -/// `read`/`write`/`list`/`delete`/`copy` are synchronous constructors in opendal's -/// `Service` trait (they build a reader/writer/etc. without performing I/O); the -/// actual I/O, and therefore any credential-expiry failure, happens later via the -/// returned `oio::Read`/`oio::Write`/etc. object, which this wrapper does not -/// control. `stat`/`create_dir`/`rename`/`presign` (still async on the trait) get -/// credential-refresh-and-retry directly via [`RefreshableAccessor::with_credential_retry`]. -/// `read` gets the same treatment indirectly: [`RetryableReader`] wraps the reader -/// returned by the current accessor and, on failure, refreshes credentials, asks a -/// freshly-rebuilt accessor for a new reader, and retries once. -/// `write`/`list`/`delete`/`copy` are not yet retry-capable: a long-lived writer, -/// lister, deleter, or copier held across a credential expiry window will surface -/// the raw error instead of transparently retrying. -pub(crate) struct RefreshableAccessor { - /// The current backend's accessor paired with the credential version it was built from. - inner: Mutex<(Servicer, u64)>, - - /// The full original path (e.g. "memory:/some-file") used to create the operator. - /// Needed to rebuild the accessor after credential refresh. - original_path: String, - - /// Shared storage holding credentials and configuration - storage: Arc, -} - -impl RefreshableAccessor { - pub(crate) fn new( - accessor: Servicer, - credential_version: u64, - original_path: String, - storage: Arc, - ) -> Self { - Self { - inner: Mutex::new((accessor, credential_version)), - original_path, - storage, - } - } - - /// Get the current inner accessor and its credential version. - fn get_accessor(&self) -> (Servicer, u64) { - let guard = self.inner.lock().unwrap(); - guard.clone() - } - - /// Rebuild the inner accessor from the shared storage after a credential refresh. - /// - /// Uses `original_path` (the full path passed to `refreshable_create_operator`) - /// to call `create_operator` on the refreshed `inner_storage`. - fn rebuild_accessor(&self, new_version: u64) -> Result { - let storage_guard = self.storage.lock_inner_storage(); - let (operator, _) = storage_guard.create_operator(&self.original_path)?; - drop(storage_guard); - - let (_ctx, new_accessor) = operator.into_parts(); - *self.inner.lock().unwrap() = (new_accessor.clone(), new_version); - Ok(new_accessor) - } - - /// Run an operation with automatic retry after credential refresh on any error. - /// - /// 1. Gets the current accessor and runs the operation. - /// 2. If it fails (any error), calls `try_refresh_credentials` with the - /// accessor's credential version. - /// 3. If credentials were refreshed (by us or another concurrent accessor), - /// rebuilds our accessor and retries the operation once. - /// 4. If the retry also fails, returns an error that preserves both the - /// original and retry error messages. - async fn with_credential_retry(&self, op: F) -> opendal::Result - where - F: Fn(Servicer) -> Fut, - Fut: Future>, - { - let (accessor, version) = self.get_accessor(); - let result = op(accessor).await; - - match result { - Err(original_err) => { - let original_display = original_err.to_string(); - let original_kind = original_err.kind(); - - let new_version = self - .storage - .try_refresh_credentials(version) - .await - .map_err(|e| { - opendal::Error::new( - original_kind, - format!( - "Operation failed and credential refresh also failed: \ - {e}. Original error: {original_display}" - ), - ) - })?; - - let new_accessor = self.rebuild_accessor(new_version).map_err(|e| { - opendal::Error::new( - opendal::ErrorKind::Unexpected, - format!( - "Failed to rebuild accessor after credential refresh. \ - Original error: {original_display}" - ), - ) - .set_source(e) - })?; - - op(new_accessor).await.map_err(|retry_err| { - opendal::Error::new( - retry_err.kind(), - format!( - "Retry after credential refresh also failed. \ - Original error: {original_display}" - ), - ) - .set_source(retry_err) - }) - } - other => other, - } - } -} - -/// Refreshes credentials (deduped against `version` via `RefreshableOpenDalStorage`'s -/// double-checked locking) and builds a fresh accessor for `original_path`, without -/// touching any particular `RefreshableAccessor`'s cached state. Used by -/// [`RetryableReader`], which tracks its own credential-version baseline independently -/// of the `RefreshableAccessor` that created it. -async fn refresh_and_build_accessor( - storage: &RefreshableOpenDalStorage, - original_path: &str, - version: u64, -) -> opendal::Result<(Servicer, u64)> { - let new_version = storage - .try_refresh_credentials(version) - .await - .map_err(|e| { - opendal::Error::new( - opendal::ErrorKind::Unexpected, - format!("Credential refresh failed: {e}"), - ) - })?; - - let storage_guard = storage.lock_inner_storage(); - let (operator, _) = storage_guard.create_operator(&original_path).map_err(|e| { - opendal::Error::new( - opendal::ErrorKind::Unexpected, - "Failed to rebuild accessor after credential refresh", - ) - .set_source(e) - })?; - drop(storage_guard); - - let (_ctx, new_accessor) = operator.into_parts(); - Ok((new_accessor, new_version)) -} - -/// A retry-capable `oio::Read` wrapper. -/// -/// opendal 0.58 made `Service::read` a synchronous constructor: it hands back a -/// reader instead of performing I/O, so `RefreshableAccessor::read` itself has -/// nothing to retry (see the "Known limitation" section on [`RefreshableAccessor`]). -/// This wrapper moves the same credential-refresh-and-rebuild strategy -/// `with_credential_retry` uses down to the reader's own `open`/`read` calls, where -/// the actual I/O -- and therefore any credential-expiry failure -- happens. -/// -/// The current inner reader lives behind a `tokio::sync::RwLock` rather than a -/// `Mutex` so that concurrent reads (e.g. parallel row-group fetches against the -/// same file) share a read lock and aren't serialized on the happy path; only a -/// failure-triggered rebuild needs the write lock, and only briefly, to swap in the -/// freshly-built reader. -struct RetryableReader { - storage: Arc, - original_path: String, - ctx: OperationContext, - path: String, - args: OpRead, - version: AtomicU64, - inner: AsyncRwLock, -} - -impl RetryableReader { - #[allow(clippy::too_many_arguments)] - fn new( - storage: Arc, - original_path: String, - ctx: OperationContext, - path: String, - args: OpRead, - version: u64, - inner: oio::Reader, - ) -> Self { - Self { - storage, - original_path, - ctx, - path, - args, - version: AtomicU64::new(version), - inner: AsyncRwLock::new(inner), - } - } - - /// Refreshes credentials, builds a fresh accessor, and asks it for a new reader - /// bound to the same path/args -- replacing the current inner reader. If multiple - /// concurrent callers each observe a failure and race to rebuild, each will build - /// its own fresh reader, but `try_refresh_credentials`'s double-checked locking - /// still ensures only one of them calls the external credential loader. - async fn refresh_and_replace(&self) -> opendal::Result<()> { - let version = self.version.load(Ordering::SeqCst); - let (new_accessor, new_version) = - refresh_and_build_accessor(&self.storage, &self.original_path, version).await?; - let new_reader = new_accessor.read(&self.ctx, &self.path, self.args.clone())?; - *self.inner.write().await = new_reader; - self.version.store(new_version, Ordering::SeqCst); - Ok(()) - } -} - -impl std::fmt::Debug for RetryableReader { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("RetryableReader").finish() - } -} - -impl OioRead for RetryableReader { - async fn read(&self, range: BytesRange) -> opendal::Result<(RpRead, Buffer)> { - let original_err = match self.inner.read().await.read(range).await { - Ok(ok) => return Ok(ok), - Err(err) => err, - }; - let original_display = original_err.to_string(); - let original_kind = original_err.kind(); - - self.refresh_and_replace().await.map_err(|e| { - opendal::Error::new( - original_kind, - format!( - "Read failed and credential refresh also failed: {e}. \ - Original error: {original_display}" - ), - ) - })?; - - self.inner - .read() - .await - .read(range) - .await - .map_err(|retry_err| { - opendal::Error::new( - retry_err.kind(), - format!( - "Retry after credential refresh also failed. \ - Original error: {original_display}" - ), - ) - .set_source(retry_err) - }) - } - - async fn open( - &self, - range: BytesRange, - ) -> opendal::Result<(RpRead, Box)> { - let original_err = match self.inner.read().await.open(range).await { - Ok(ok) => return Ok(ok), - Err(err) => err, - }; - let original_display = original_err.to_string(); - let original_kind = original_err.kind(); - - self.refresh_and_replace().await.map_err(|e| { - opendal::Error::new( - original_kind, - format!( - "Open failed and credential refresh also failed: {e}. \ - Original error: {original_display}" - ), - ) - })?; - - self.inner - .read() - .await - .open(range) - .await - .map_err(|retry_err| { - opendal::Error::new( - retry_err.kind(), - format!( - "Retry after credential refresh also failed. \ - Original error: {original_display}" - ), - ) - .set_source(retry_err) - }) - } -} - -impl std::fmt::Debug for RefreshableAccessor { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("RefreshableAccessor").finish() - } -} - -impl Service for RefreshableAccessor { - type Reader = oio::Reader; - type Writer = oio::Writer; - type Lister = oio::Lister; - type Deleter = oio::Deleter; - type Copier = oio::Copier; - - fn info(&self) -> ServiceInfo { - let info_guard = self.storage.lock_cached_info(); - if let Some(info) = info_guard.as_ref() { - info.clone() - } else { - drop(info_guard); - ServiceInfo::with_scheme("") - } - } - - fn capability(&self) -> Capability { - let (accessor, _) = self.get_accessor(); - accessor.capability() - } - - async fn stat( - &self, - ctx: &OperationContext, - path: &str, - args: OpStat, - ) -> opendal::Result { - self.with_credential_retry(|accessor| { - let args = args.clone(); - async move { accessor.stat(ctx, path, args).await } - }) - .await - } - - // `read` returns a retry-capable `RetryableReader` (see the "Known limitation" - // doc comment above and `RetryableReader`'s own doc comment). `write`/`list`/ - // `delete`/`copy` are synchronous constructors in opendal's `Service` trait: they - // build a writer/lister/etc. without performing I/O, so there is no failure here - // to retry against; they delegate directly to the current accessor. - fn read( - &self, - ctx: &OperationContext, - path: &str, - args: OpRead, - ) -> opendal::Result { - let (accessor, version) = self.get_accessor(); - let inner = accessor.read(ctx, path, args.clone())?; - Ok(Box::new(RetryableReader::new( - self.storage.clone(), - self.original_path.clone(), - ctx.clone(), - path.to_string(), - args, - version, - inner, - ))) - } - - fn write( - &self, - ctx: &OperationContext, - path: &str, - args: OpWrite, - ) -> opendal::Result { - let (accessor, _) = self.get_accessor(); - accessor.write(ctx, path, args) - } - - fn delete(&self, ctx: &OperationContext) -> opendal::Result { - let (accessor, _) = self.get_accessor(); - accessor.delete(ctx) - } - - fn list( - &self, - ctx: &OperationContext, - path: &str, - args: OpList, - ) -> opendal::Result { - let (accessor, _) = self.get_accessor(); - accessor.list(ctx, path, args) - } - - fn copy( - &self, - ctx: &OperationContext, - from: &str, - to: &str, - args: OpCopy, - opts: OpCopier, - ) -> opendal::Result { - let (accessor, _) = self.get_accessor(); - accessor.copy(ctx, from, to, args, opts) - } - - async fn create_dir( - &self, - ctx: &OperationContext, - path: &str, - args: OpCreateDir, - ) -> opendal::Result { - self.with_credential_retry(|accessor| { - let args = args.clone(); - async move { accessor.create_dir(ctx, path, args).await } - }) - .await - } - - async fn rename( - &self, - ctx: &OperationContext, - from: &str, - to: &str, - args: OpRename, - ) -> opendal::Result { - self.with_credential_retry(|accessor| { - let args = args.clone(); - async move { accessor.rename(ctx, from, to, args).await } - }) - .await - } - - async fn presign( - &self, - ctx: &OperationContext, - path: &str, - args: OpPresign, - ) -> opendal::Result { - self.with_credential_retry(|accessor| { - let args = args.clone(); - async move { accessor.presign(ctx, path, args).await } - }) - .await - } -} - -/// Tests for the `with_credential_retry` logic in `RefreshableAccessor`. -/// -/// `with_credential_retry` works as follows: -/// 1. Gets the current accessor (no refresh) and runs the operation. -/// 2. On any error, calls `try_refresh_credentials` with the accessor's -/// credential version. -/// 3. If credentials were refreshed, rebuilds the accessor and retries once. -/// 4. If the retry also fails, returns an error preserving both original and -/// retry error messages. -/// -/// To test this, we inject a `FailingAccessor` (returns a configurable error on `stat`) -/// as the initial inner accessor, while the shared storage's `inner_storage` is a real -/// memory backend. When credential refresh triggers a rebuild, the accessor switches -/// from `FailingAccessor` to the real memory backend — observable as a change in error -/// kind (e.g. `PermissionDenied` → `NotFound`). -/// -/// A `SequenceLoader` controls exactly which loader calls trigger refresh (`Some`) and -/// which don't (`None`), so we can test each branch of the retry logic. -#[cfg(all(test, feature = "storage-memory"))] -mod tests { - use std::collections::{HashMap, VecDeque}; - use std::sync::atomic::{AtomicUsize, Ordering}; - - use super::*; - use crate::NamespaceIdent; - use crate::catalog::TableIdent; - use crate::io::refreshable_storage::RefreshableOpenDalStorageBuilder; - use crate::io::{StorageCredential, StorageCredentialsLoader}; - - // --- Test helpers --- - - /// Returns pre-configured credentials in order from a `VecDeque`. Tracks call count. - struct SequenceLoader { - responses: Mutex>, - call_count: AtomicUsize, - } - - impl std::fmt::Debug for SequenceLoader { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("SequenceLoader").finish() - } - } - - impl SequenceLoader { - fn new(responses: Vec) -> Self { - Self { - responses: Mutex::new(VecDeque::from(responses)), - call_count: AtomicUsize::new(0), - } - } - - fn call_count(&self) -> usize { - self.call_count.load(Ordering::SeqCst) - } - } - - #[async_trait::async_trait] - impl StorageCredentialsLoader for SequenceLoader { - async fn load_credentials( - &self, - _table_ident: &TableIdent, - _location: &str, - ) -> Result { - self.call_count.fetch_add(1, Ordering::SeqCst); - let mut responses = self.responses.lock().unwrap(); - Ok(responses.pop_front().unwrap_or_else(dummy_credential)) - } - } - - /// An `oio::Read` that always fails with a configurable error kind. Models how a - /// real backend like S3 behaves: the reader *constructs* successfully (no I/O at - /// that point), and the credential failure only surfaces once `read`/`open` - /// actually issues a request. - struct FailingReader { - error_kind: opendal::ErrorKind, - } - - impl OioRead for FailingReader { - async fn read(&self, _range: BytesRange) -> opendal::Result<(RpRead, Buffer)> { - Err(opendal::Error::new(self.error_kind, "test read error")) - } - - async fn open( - &self, - _range: BytesRange, - ) -> opendal::Result<(RpRead, Box)> { - Err(opendal::Error::new(self.error_kind, "test open error")) - } - } - - /// `Service` impl that always returns a configurable `opendal::ErrorKind` on `stat`, - /// and whose `read` constructs a `FailingReader` that fails the same way on the - /// actual I/O call. All other methods return `Unexpected` (not expected to be - /// called by these tests). - struct FailingAccessor { - error_kind: opendal::ErrorKind, - info: ServiceInfo, - } - - impl std::fmt::Debug for FailingAccessor { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("FailingAccessor").finish() - } - } - - impl FailingAccessor { - fn new(error_kind: opendal::ErrorKind, info: ServiceInfo) -> Self { - Self { error_kind, info } - } - - fn unexpected() -> opendal::Result { - Err(opendal::Error::new( - opendal::ErrorKind::Unexpected, - "not implemented in test", - )) - } - } - - impl Service for FailingAccessor { - type Reader = oio::Reader; - type Writer = oio::Writer; - type Lister = oio::Lister; - type Deleter = oio::Deleter; - type Copier = oio::Copier; - - fn info(&self) -> ServiceInfo { - self.info.clone() - } - - fn capability(&self) -> Capability { - Capability::default() - } - - async fn stat( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpStat, - ) -> opendal::Result { - Err(opendal::Error::new(self.error_kind, "test error")) - } - - fn read( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpRead, - ) -> opendal::Result { - Ok(Box::new(FailingReader { - error_kind: self.error_kind, - })) - } - - fn write( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpWrite, - ) -> opendal::Result { - Self::unexpected() - } - - fn delete(&self, _ctx: &OperationContext) -> opendal::Result { - Self::unexpected() - } - - fn list( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpList, - ) -> opendal::Result { - Self::unexpected() - } - - fn copy( - &self, - _ctx: &OperationContext, - _from: &str, - _to: &str, - _args: OpCopy, - _opts: OpCopier, - ) -> opendal::Result { - Self::unexpected() - } - - async fn create_dir( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpCreateDir, - ) -> opendal::Result { - Self::unexpected() - } - - async fn rename( - &self, - _ctx: &OperationContext, - _from: &str, - _to: &str, - _args: OpRename, - ) -> opendal::Result { - Self::unexpected() - } - - async fn presign( - &self, - _ctx: &OperationContext, - _path: &str, - _args: OpPresign, - ) -> opendal::Result { - Self::unexpected() - } - } - - fn dummy_credential() -> StorageCredential { - StorageCredential { - prefix: "memory:/".to_string(), - config: HashMap::from([("dummy".to_string(), "cred".to_string())]), - } - } - - /// Builds a `RefreshableAccessor` whose initial inner accessor is a `FailingAccessor` - /// (returns `error_kind` on stat), but whose shared storage is a real memory backend. - /// After credential refresh + rebuild, the accessor switches from `FailingAccessor` - /// to the real memory backend. - fn build_refreshable_storage_and_accessor( - loader: Arc, - error_kind: opendal::ErrorKind, - ) -> RefreshableAccessor { - let storage = RefreshableOpenDalStorageBuilder::new() - .scheme("memory".to_string()) - .base_props(HashMap::new()) - .credentials_loader(Arc::clone(&loader)) - .table_ident(TableIdent::new( - NamespaceIdent::new("test_ns".to_string()), - "test_table".to_string(), - )) - .build() - .expect("Failed to build storage"); - - let info = { - let inner = storage.lock_inner_storage(); - let path = "memory:/dummy".to_string(); - let (op, _) = inner.create_operator(&path).unwrap(); - let (_ctx, accessor) = op.into_parts(); - accessor.info() - }; - - *storage.lock_cached_info() = Some(info.clone()); - - let version = storage.credential_version(); - let failing_accessor: Servicer = Arc::new(FailingAccessor::new(error_kind, info)); - RefreshableAccessor::new( - failing_accessor, - version, - "memory:/dummy".to_string(), - storage, - ) - } - - // --- Tests --- - - /// Core retry scenario: when temporary credentials expire mid-operation, - /// the accessor should transparently refresh and retry. - /// - /// Flow: - /// 1. `get_accessor` → no refresh → FailingAccessor used - /// 2. `stat` → PermissionDenied - /// 3. `try_refresh_credentials` → loader call #1 → do_refresh - /// 4. `rebuild_accessor` → memory accessor used - /// 5. Memory backend `stat("nonexistent")` → NotFound (not PermissionDenied) - #[tokio::test] - async fn test_retry_after_credential_refresh() { - let loader = Arc::new(SequenceLoader::new(vec![dummy_credential()])); - - let accessor = build_refreshable_storage_and_accessor( - Arc::clone(&loader) as _, - opendal::ErrorKind::PermissionDenied, - ); - - let result = accessor - .stat(&OperationContext::new(), "nonexistent", OpStat::new()) - .await; - - // The retry should have happened — the error should be NotFound - // (from the memory backend), not PermissionDenied - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!( - err.kind(), - opendal::ErrorKind::NotFound, - "Expected NotFound after retry, got {:?}", - err.kind() - ); - - // Only 1 loader call: the retry on PermissionDenied - assert_eq!(loader.call_count(), 1); - } - - /// Same scenario as `test_retry_after_credential_refresh`, but for `read`. Unlike - /// `stat`, `read` is a synchronous constructor on the `Service` trait — the failure - /// and retry happen inside the returned `RetryableReader`'s `oio::Read::read` call, - /// not in `RefreshableAccessor::read` itself. - /// - /// Flow: - /// 1. `RefreshableAccessor::read` → `FailingAccessor::read` constructs a - /// `FailingReader`, wrapped in a `RetryableReader` — no failure yet. - /// 2. `RetryableReader::read` → delegates to `FailingReader::read` → PermissionDenied - /// 3. `try_refresh_credentials` → loader call #1 → do_refresh - /// 4. `refresh_and_replace` rebuilds against the memory backend and asks it for a - /// fresh reader, replacing the failing one - /// 5. Memory backend reader `read("nonexistent")` → NotFound (not PermissionDenied) - #[tokio::test] - async fn test_read_retries_after_credential_refresh() { - let loader = Arc::new(SequenceLoader::new(vec![dummy_credential()])); - - let accessor = build_refreshable_storage_and_accessor( - Arc::clone(&loader) as _, - opendal::ErrorKind::PermissionDenied, - ); - - let reader = accessor - .read(&OperationContext::new(), "nonexistent", OpRead::new()) - .expect("constructing the reader should not fail"); - - // A bounded range: the memory backend's `StreamReader::read` requires one - // (an unbounded range must go through `open` instead). - let result = reader.read(BytesRange::new(0, Some(1))).await; - - // The retry should have happened — the error should be NotFound - // (from the memory backend), not PermissionDenied - assert!(result.is_err()); - let err = result.unwrap_err(); - assert_eq!( - err.kind(), - opendal::ErrorKind::NotFound, - "Expected NotFound after retry, got {:?}", - err.kind() - ); - - // Only 1 loader call: the retry on PermissionDenied - assert_eq!(loader.call_count(), 1); - } - - /// Any error triggers credential retry. When both the original and retry - /// operations fail, the error message should preserve both. - /// - /// Flow: - /// 1. `get_accessor` → FailingAccessor → Unexpected error - /// 2. `try_refresh_credentials` → loader call → do_refresh - /// 3. `rebuild_accessor` → memory accessor - /// 4. Memory backend `stat("nonexistent")` → NotFound - /// 5. Final error includes both "Unexpected" original and "NotFound" retry info - #[tokio::test] - async fn test_any_error_triggers_retry_and_preserves_both_errors() { - let loader = Arc::new(SequenceLoader::new(vec![dummy_credential()])); - - let accessor = build_refreshable_storage_and_accessor( - Arc::clone(&loader) as _, - opendal::ErrorKind::Unexpected, - ); - - let result = accessor - .stat(&OperationContext::new(), "nonexistent", OpStat::new()) - .await; - - assert!(result.is_err()); - let err = result.unwrap_err(); - // The retry error kind comes from the memory backend (NotFound) - assert_eq!( - err.kind(), - opendal::ErrorKind::NotFound, - "Expected NotFound from retry, got {:?}", - err.kind() - ); - - // Error message should mention both the retry failure and original error - let err_msg = err.to_string(); - assert!( - err_msg.contains("Original error"), - "Error should reference original error: {err_msg}" - ); - assert!( - err_msg.contains("Unexpected"), - "Error should contain original Unexpected error kind: {err_msg}" - ); - - // 1 loader call — retry happened - assert_eq!(loader.call_count(), 1); - } - - /// When multiple concurrent callers trigger credential refresh, only one - /// should call the external credential loader. The others should detect the - /// version bump and skip the loader call. - #[tokio::test] - async fn test_concurrent_refresh_calls_loader_only_once() { - let loader = Arc::new(SequenceLoader::new(vec![dummy_credential()])); - - let storage = RefreshableOpenDalStorageBuilder::new() - .scheme("memory".to_string()) - .base_props(HashMap::new()) - .credentials_loader(Arc::clone(&loader) as _) - .table_ident(TableIdent::new( - NamespaceIdent::new("test_ns".to_string()), - "test_table".to_string(), - )) - .build() - .expect("Failed to build storage"); - - let version = storage.credential_version(); - - // Spawn 10 concurrent try_refresh_credentials calls with the same version - let mut handles = Vec::new(); - for _ in 0..10 { - let storage = Arc::clone(&storage); - handles.push(tokio::spawn(async move { - storage.try_refresh_credentials(version).await - })); - } - - for handle in handles { - let new_version = handle.await.unwrap().unwrap(); - assert_eq!(new_version, 1, "Version should be 1 after one refresh"); - } - - // Only 1 loader call should have been made - assert_eq!(loader.call_count(), 1); - } -} diff --git a/crates/iceberg/src/io/refreshable_storage.rs b/crates/iceberg/src/io/refreshable_storage.rs deleted file mode 100644 index 495db6e67f..0000000000 --- a/crates/iceberg/src/io/refreshable_storage.rs +++ /dev/null @@ -1,449 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::collections::HashMap; -use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::{Arc, Mutex, MutexGuard}; - -use opendal::Operator; -use opendal::raw::*; -use tokio::sync::Mutex as AsyncMutex; - -use super::refreshable_accessor::RefreshableAccessor; -use super::storage::opendal::OpenDalStorage; -use crate::catalog::TableIdent; -use crate::io::{StorageCredential, StorageCredentialsLoader}; -use crate::{Error, ErrorKind, Result}; - -/// Holds shared configuration and state for credential refresh. -/// -/// Multiple `RefreshableAccessor` instances share a single `RefreshableOpenDalStorage` -/// via `Arc`, allowing credential refreshes to be visible across all accessors. -pub struct RefreshableOpenDalStorage { - /// Scheme of the inner backend (e.g., "s3", "azdls") - scheme: String, - - /// Base properties (non-credential config like endpoint, region, etc.) - base_props: HashMap, - - /// Inner storage (built in new, rebuilt on credential refresh) - inner_storage: Mutex, - - /// Credential loader - credentials_loader: Arc, - - /// Metadata location passed to `load_credentials` - location: String, - - /// Table identifier passed to `load_credentials` - table_ident: TableIdent, - - /// Cached ServiceInfo (created lazily from first operator) - cached_info: Mutex>, - - /// Monotonically increasing version number, incremented each time credentials - /// are refreshed via do_refresh. Used by RefreshableAccessor instances to detect - /// whether someone else has already refreshed since their accessor was built. - credential_version: AtomicU64, - - /// Async mutex that serializes calls to the external credential loader. - /// Held across the await point of maybe_load_credentials to ensure only - /// one concurrent caller invokes the loader at a time. - refresh_lock: AsyncMutex<()>, -} - -impl std::fmt::Debug for RefreshableOpenDalStorage { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("RefreshableOpenDalStorage").finish() - } -} - -impl RefreshableOpenDalStorage { - /// Creates a new RefreshableOpenDalStorage. - /// - /// # Arguments - /// * `scheme` - Storage scheme (e.g., "s3", "azdls") - /// * `base_props` - Base configuration properties (including any initial credentials) - /// * `credentials_loader` - Loader for refreshing credentials - /// * `location` - Metadata location passed to `load_credentials` - /// * `table_ident` - Table identifier passed to `load_credentials` - pub fn new( - scheme: String, - base_props: HashMap, - credentials_loader: Arc, - location: String, - table_ident: TableIdent, - ) -> Result { - let inner_storage = OpenDalStorage::build_from_props(&scheme, base_props.clone())?; - - Ok(Self { - scheme, - base_props, - inner_storage: Mutex::new(inner_storage), - credentials_loader, - location, - table_ident, - cached_info: Mutex::new(None), - credential_version: AtomicU64::new(0), - refresh_lock: AsyncMutex::new(()), - }) - } - - /// Create an operator for the given path. - /// - /// Builds a `RefreshableAccessor` that wraps the inner storage operator and - /// delegates all operations through credential refresh logic. - pub fn refreshable_create_operator(self: &Arc, path: &str) -> Result<(Operator, String)> { - let storage_guard = self.lock_inner_storage(); - let path_string = path.to_string(); - let (operator, relative_path) = storage_guard.create_operator(&path_string)?; - let relative_path = relative_path.to_string(); - // Read version while still holding the lock so the accessor and version - // are guaranteed to be consistent. Otherwise a concurrent do_refresh - // could bump the version between dropping the lock and reading it, - // causing the accessor to appear up-to-date when it's actually stale. - let version = self.credential_version(); - drop(storage_guard); - - let (ctx, accessor) = operator.into_parts(); - - // Cache ServiceInfo if not already cached - { - let mut info_guard = self.lock_cached_info(); - if info_guard.is_none() { - *info_guard = Some(accessor.info()); - } - } - let refreshable_accessor = - RefreshableAccessor::new(accessor, version, path.to_string(), Arc::clone(self)); - - let wrapped_operator = Operator::from_parts(ctx, Arc::new(refreshable_accessor)); - Ok((wrapped_operator, relative_path)) - } - - /// Rebuild inner storage from new credentials and bump the credential version. - fn do_refresh(&self, new_creds: StorageCredential) -> Result<()> { - let mut full_props = self.base_props.clone(); - full_props.extend(new_creds.config.clone()); - - let new_storage = OpenDalStorage::build_from_props(&self.scheme, full_props)?; - - // Update storage and bump version atomically (while holding the lock) - // so that refreshable_create_operator always sees a consistent - // (storage, version) pair. - let mut guard = self.lock_inner_storage(); - *guard = new_storage; - self.credential_version.fetch_add(1, Ordering::Release); - drop(guard); - - Ok(()) - } - - /// Returns the current credential version number. - pub(crate) fn credential_version(&self) -> u64 { - self.credential_version.load(Ordering::Acquire) - } - - /// Lock and return the inner storage guard. - pub(crate) fn lock_inner_storage(&self) -> MutexGuard<'_, OpenDalStorage> { - self.inner_storage.lock().unwrap() - } - - /// Lock and return the cached info guard. - pub(crate) fn lock_cached_info(&self) -> MutexGuard<'_, Option> { - self.cached_info.lock().unwrap() - } - - /// Try to refresh credentials after an operation failure. - /// - /// Uses double-checked locking with a version number: - /// 1. If `credential_version > accessor_version`, someone already refreshed — - /// return the current version without calling the loader. - /// 2. Acquire `refresh_lock` (async, serializes loader calls). - /// 3. Double-check: if `credential_version > accessor_version`, another caller - /// already refreshed while we waited — return the current version. - /// 4. Call the loader, then `do_refresh`. - /// 5. Return the new version. - pub(crate) async fn try_refresh_credentials(&self, accessor_version: u64) -> Result { - // Fast path: someone already refreshed since this accessor was built - let current = self.credential_version.load(Ordering::Acquire); - if current > accessor_version { - return Ok(current); - } - - // Acquire the async lock to serialize loader calls - let _guard = self.refresh_lock.lock().await; - - // Double-check after acquiring lock - let current = self.credential_version.load(Ordering::Acquire); - if current > accessor_version { - return Ok(current); - } - - // We are the one who should call the loader - let new_creds = self - .credentials_loader - .load_credentials(&self.table_ident, &self.location) - .await?; - self.do_refresh(new_creds)?; - Ok(self.credential_version.load(Ordering::Acquire)) - } -} - -/// Builder for RefreshableOpenDalStorage -#[derive(Default, Debug)] -pub struct RefreshableOpenDalStorageBuilder { - scheme: Option, - base_props: HashMap, - credentials_loader: Option>, - location: String, - table_ident: Option, -} - -impl RefreshableOpenDalStorageBuilder { - /// Create a new builder - pub fn new() -> Self { - Self::default() - } - - /// Set the storage scheme (e.g., "s3", "azdls") - pub fn scheme(mut self, scheme: String) -> Self { - self.scheme = Some(scheme); - self - } - - /// Set the base properties (non-credential configuration) - pub fn base_props(mut self, props: HashMap) -> Self { - self.base_props = props; - self - } - - /// Set the credentials loader - pub fn credentials_loader(mut self, loader: Arc) -> Self { - self.credentials_loader = Some(loader); - self - } - - /// Set the metadata location passed to `load_credentials` - pub fn location(mut self, location: String) -> Self { - self.location = location; - self - } - - /// Set the table identifier passed to `load_credentials` - pub fn table_ident(mut self, table_ident: TableIdent) -> Self { - self.table_ident = Some(table_ident); - self - } - - /// Build the RefreshableOpenDalStorage wrapped in Arc - pub fn build(self) -> Result> { - Ok(Arc::new(RefreshableOpenDalStorage::new( - self.scheme - .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "scheme is required"))?, - self.base_props, - self.credentials_loader.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "credentials_loader is required") - })?, - self.location, - self.table_ident - .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "table_ident is required"))?, - )?)) - } -} - -#[cfg(all(test, feature = "storage-memory"))] -mod tests { - use std::sync::atomic::{AtomicUsize, Ordering}; - - use super::*; - use crate::NamespaceIdent; - use crate::io::StorageCredential; - - // --- Test helpers --- - - /// Simple loader that always returns the same credential. - #[derive(Debug)] - struct SimpleLoader; - - #[async_trait::async_trait] - impl StorageCredentialsLoader for SimpleLoader { - async fn load_credentials( - &self, - _table_ident: &TableIdent, - _location: &str, - ) -> Result { - Ok(StorageCredential { - prefix: "memory:/refreshed/".to_string(), - config: HashMap::from([("refreshed_key".to_string(), "refreshed_val".to_string())]), - }) - } - } - - /// Records every call: increments a counter. - /// Returns a credential whose `config` contains the call number (e.g. `{"call": "1"}`). - struct TrackingRefreshLoader { - call_count: AtomicUsize, - } - - impl std::fmt::Debug for TrackingRefreshLoader { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("TrackingRefreshLoader").finish() - } - } - - impl TrackingRefreshLoader { - fn new() -> Self { - Self { - call_count: AtomicUsize::new(0), - } - } - - fn call_count(&self) -> usize { - self.call_count.load(Ordering::SeqCst) - } - } - - #[async_trait::async_trait] - impl StorageCredentialsLoader for TrackingRefreshLoader { - async fn load_credentials( - &self, - _table_ident: &TableIdent, - _location: &str, - ) -> Result { - let n = self.call_count.fetch_add(1, Ordering::SeqCst) + 1; - Ok(StorageCredential { - prefix: format!("memory:/refresh-{n}/"), - config: HashMap::from([("call".to_string(), n.to_string())]), - }) - } - } - - fn test_table_ident() -> TableIdent { - TableIdent::new( - NamespaceIdent::new("test_ns".to_string()), - "test_table".to_string(), - ) - } - - fn build_memory_refreshable( - loader: Arc, - ) -> Arc { - RefreshableOpenDalStorageBuilder::new() - .scheme("memory".to_string()) - .base_props(HashMap::new()) - .credentials_loader(loader) - .table_ident(test_table_ident()) - .build() - .expect("Failed to build RefreshableOpenDalStorage for memory") - } - - /// Load credentials and refresh inner storage. Test-only helper. - async fn refresh(storage: &RefreshableOpenDalStorage) -> Result<()> { - let new_creds = storage - .credentials_loader - .load_credentials(&storage.table_ident, &storage.location) - .await?; - storage.do_refresh(new_creds) - } - - // --- Tests --- - - /// Verifies `refresh` calls the loader and rebuilds inner storage. - #[tokio::test] - async fn test_refresh_calls_loader() { - let loader = Arc::new(TrackingRefreshLoader::new()); - let storage = build_memory_refreshable(Arc::clone(&loader) as _); - - refresh(&storage).await.unwrap(); - assert_eq!(loader.call_count(), 1); - - refresh(&storage).await.unwrap(); - assert_eq!(loader.call_count(), 2); - } - - /// Verifies that `do_refresh` rebuilds `inner_storage` with a fresh instance - /// (data written to the old storage is gone after refresh). - #[tokio::test] - async fn test_do_refresh_rebuilds_inner_storage() { - let storage = build_memory_refreshable(Arc::new(SimpleLoader)); - - // Write data via the current inner storage - let path = "memory:/test-file".to_string(); - { - let (op, rel) = { - let inner = storage.lock_inner_storage(); - inner.create_operator(&path).unwrap() - }; - op.write(rel, bytes::Bytes::from("hello")).await.unwrap(); - - // Verify the data is there - let (op2, rel2) = { - let inner = storage.lock_inner_storage(); - inner.create_operator(&path).unwrap() - }; - let data = op2.read(rel2).await.unwrap().to_bytes(); - assert_eq!(data, bytes::Bytes::from("hello")); - } - - // Refresh credentials — this rebuilds inner_storage with a fresh memory backend - refresh(&storage).await.unwrap(); - - // The new inner storage is a fresh memory instance; old data should be gone - let (op3, rel3) = { - let inner = storage.lock_inner_storage(); - inner.create_operator(&path).unwrap() - }; - let exists = op3.exists(rel3).await.unwrap(); - assert!( - !exists, - "Data from old storage should not exist after rebuild" - ); - } - - /// Verifies that `credential_version` increments on each `do_refresh` (via `refresh`). - #[tokio::test] - async fn test_credential_version_increments_on_refresh() { - let storage = build_memory_refreshable(Arc::new(SimpleLoader)); - - assert_eq!(storage.credential_version(), 0); - - refresh(&storage).await.unwrap(); - assert_eq!(storage.credential_version(), 1); - - refresh(&storage).await.unwrap(); - assert_eq!(storage.credential_version(), 2); - } - - /// End-to-end sanity check that `refreshable_create_operator` produces a working - /// `Operator` that wraps the inner storage correctly via `RefreshableAccessor`. - #[tokio::test] - async fn test_refreshable_operator_can_write_and_read() { - let storage = build_memory_refreshable(Arc::new(SimpleLoader)); - - let (op, rel) = storage - .refreshable_create_operator("memory:/roundtrip-file") - .unwrap(); - - op.write(&rel, bytes::Bytes::from("roundtrip data")) - .await - .unwrap(); - - let read_back = op.read(&rel).await.unwrap().to_bytes(); - assert_eq!(read_back, bytes::Bytes::from("roundtrip data")); - } -} diff --git a/crates/iceberg/src/io/storage/mod.rs b/crates/iceberg/src/io/storage/mod.rs index 97e612f5bc..d4c0e3fa5c 100644 --- a/crates/iceberg/src/io/storage/mod.rs +++ b/crates/iceberg/src/io/storage/mod.rs @@ -20,7 +20,6 @@ pub mod config; mod local_fs; mod memory; -pub mod opendal; use std::fmt::Debug; use std::sync::Arc; @@ -31,11 +30,6 @@ pub use config::*; use futures::stream::BoxStream; pub use local_fs::{LocalFsStorage, LocalFsStorageFactory}; pub use memory::{MemoryStorage, MemoryStorageFactory}; -#[cfg(feature = "storage-s3")] -pub use opendal::CustomAwsCredentialLoader; -pub use opendal::{ - OpenDalRoutingStorageFactory, OpenDalStorage, OpenDalStorageFactory, RefreshableStorageFactory, -}; use super::{FileMetadata, FileRead, FileWrite, InputFile, OutputFile}; use crate::Result; diff --git a/crates/iceberg/src/io/storage/opendal/azdls.rs b/crates/iceberg/src/io/storage/opendal/azdls.rs deleted file mode 100644 index 4b9e90fb0d..0000000000 --- a/crates/iceberg/src/io/storage/opendal/azdls.rs +++ /dev/null @@ -1,766 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::collections::HashMap; -use std::fmt::Display; -use std::str::FromStr; - -use opendal::Configurator; -use opendal::services::AzdlsConfig; -use serde::{Deserialize, Serialize}; -use url::Url; - -use super::super::config::{ - ADLS_ACCOUNT_KEY, ADLS_ACCOUNT_NAME, ADLS_AUTHORITY_HOST, ADLS_CLIENT_ID, ADLS_CLIENT_SECRET, - ADLS_CONNECTION_STRING, ADLS_ENDPOINT, ADLS_TENANT_ID, find_sas_token, -}; -use crate::{Error, ErrorKind, Result, ensure_data_valid}; - -/// Parses adls.* prefixed configuration properties. -pub(crate) fn azdls_config_parse(mut properties: HashMap) -> Result { - let mut config = AzdlsConfig::default(); - - if let Some(_conn_str) = properties.remove(ADLS_CONNECTION_STRING) { - return Err(Error::new( - ErrorKind::FeatureUnsupported, - "Azdls: connection string currently not supported", - )); - } - - if let Some(endpoint) = properties.remove(ADLS_ENDPOINT) { - config.endpoint = Some(endpoint); - } - - if let Some(account_name) = properties.remove(ADLS_ACCOUNT_NAME) { - config.account_name = Some(account_name); - } - - if let Some(account_key) = properties.remove(ADLS_ACCOUNT_KEY) { - config.account_key = Some(account_key); - } - - if let Some(sas_token) = find_sas_token(&properties, config.account_name.as_deref()) { - config.sas_token = Some(sas_token); - } - - if let Some(tenant_id) = properties.remove(ADLS_TENANT_ID) { - config.tenant_id = Some(tenant_id); - } - - if let Some(client_id) = properties.remove(ADLS_CLIENT_ID) { - config.client_id = Some(client_id); - } - - if let Some(client_secret) = properties.remove(ADLS_CLIENT_SECRET) { - config.client_secret = Some(client_secret); - } - - if let Some(authority_host) = properties.remove(ADLS_AUTHORITY_HOST) { - config.authority_host = Some(authority_host); - } - - Ok(config) -} - -/// Builds an OpenDAL operator from the AzdlsConfig and path. -/// -/// The path is expected to include the scheme in a format like: -/// `abfss://@.dfs.core.windows.net/mydir/myfile.parquet`. -pub(crate) fn azdls_create_operator<'a>( - absolute_path: &'a str, - config: &AzdlsConfig, - configured_scheme: &AzureStorageScheme, - allow_anonymous: bool, -) -> Result<(opendal::Operator, &'a str)> { - let path = absolute_path.parse::()?; - match_path_with_config(&path, config, configured_scheme)?; - - let op = azdls_config_build(config, &path, allow_anonymous)?; - - // Paths to files in ADLS tend to be written in fully qualified form, - // including their filesystem and account name. - // OpenDAL's operator methods expect only the relative path, so we split it - // off and save it for later use. - let relative_path_len = path.path.len(); - let (_, relative_path) = absolute_path.split_at(absolute_path.len() - relative_path_len); - - Ok((op, relative_path)) -} - -/// Note that `abf[s]` and `wasb[s]` variants have different implications: -/// - `abfs[s]` is used to refer to files in ADLS Gen2, backed by blob storage; -/// paths are expected to contain the `dfs` storage service. -/// - `wasb[s]` is used to refer to files in Blob Storage directly; paths are -/// expected to contain the `blob` storage service. -#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] -pub enum AzureStorageScheme { - Abfs, - Abfss, - Wasb, - Wasbs, -} - -impl AzureStorageScheme { - // Returns the respective encrypted or plain-text HTTP scheme. - pub fn as_http_scheme(&self) -> &str { - match self { - AzureStorageScheme::Abfs | AzureStorageScheme::Wasb => "http", - AzureStorageScheme::Abfss | AzureStorageScheme::Wasbs => "https", - } - } -} - -impl Display for AzureStorageScheme { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AzureStorageScheme::Abfs => write!(f, "abfs"), - AzureStorageScheme::Abfss => write!(f, "abfss"), - AzureStorageScheme::Wasb => write!(f, "wasb"), - AzureStorageScheme::Wasbs => write!(f, "wasbs"), - } - } -} - -impl FromStr for AzureStorageScheme { - type Err = Error; - - fn from_str(s: &str) -> Result { - match s { - "abfs" => Ok(AzureStorageScheme::Abfs), - "abfss" => Ok(AzureStorageScheme::Abfss), - "wasb" => Ok(AzureStorageScheme::Wasb), - "wasbs" => Ok(AzureStorageScheme::Wasbs), - _ => Err(Error::new( - ErrorKind::DataInvalid, - format!("Unexpected Azure Storage scheme: {s}"), - )), - } - } -} - -/// Validates whether the given path matches what's configured for the backend. -fn match_path_with_config( - path: &AzureStoragePath, - config: &AzdlsConfig, - configured_scheme: &AzureStorageScheme, -) -> Result<()> { - ensure_data_valid!( - &path.scheme == configured_scheme, - "Storage::Azdls: Scheme mismatch: configured {}, passed {}", - configured_scheme, - path.scheme - ); - - if let Some(ref configured_account_name) = config.account_name { - ensure_data_valid!( - &path.account_name == configured_account_name, - "Storage::Azdls: Account name mismatch: configured {}, path {}", - configured_account_name, - path.account_name - ); - } - - if let Some(ref configured_endpoint) = config.endpoint { - let passed_http_scheme = path.scheme.as_http_scheme(); - ensure_data_valid!( - configured_endpoint.starts_with(passed_http_scheme), - "Storage::Azdls: Endpoint {} does not use the expected http scheme {}.", - configured_endpoint, - passed_http_scheme - ); - - // Skip endpoint suffix check for local endpoints (e.g., Azurite) - let is_local = - configured_endpoint.contains("127.0.0.1") || configured_endpoint.contains("localhost"); - if !is_local { - let ends_with_expected_suffix = configured_endpoint - .trim_end_matches('/') - .ends_with(&path.endpoint_suffix); - ensure_data_valid!( - ends_with_expected_suffix, - "Storage::Azdls: Endpoint suffix {} used with configured endpoint {}.", - path.endpoint_suffix, - configured_endpoint, - ); - } - } - - Ok(()) -} - -fn azdls_config_build( - config: &AzdlsConfig, - path: &AzureStoragePath, - allow_anonymous: bool, -) -> Result { - // opendal's Azdls service (via reqsign-azure-storage) has no anonymous/ - // unsigned mode: every request goes through a generic credential-provider - // chain whose `Credential` variants (SharedKey/SasToken/BearerToken) all - // require a non-empty secret to be considered valid, and the signer hard - // errors ("failed to load signing credential") if none can be found or - // validated -- unlike e.g. its S3 service, which exposes an explicit - // `skip_signature` bypass. So when no credential was configured at all, - // we can't get a genuinely anonymous request out of the Azdls service no - // matter what we pass it (an empty SAS token doesn't help either: it's - // treated as a *found but invalid* credential, which fails the same way). - // - // Public/anonymous-read Azure containers are plain, unauthenticated HTTPS - // (GET with a Range header for data, HEAD for stat) -- exactly what - // opendal's generic `Http` service already provides, with no built-in - // auth of its own unless a username/password/token is set. So route the - // no-credentials case there instead, scoped to the same account/container - // endpoint the Azdls builder would have used. - // - // This is opt-in via `allow_anonymous` (see `ADLS_ALLOW_ANONYMOUS`), not - // inferred from "no credentials configured": the absence of an explicit - // adls.* credential can also legitimately mean "rely on ambient Azure - // credentials" (managed identity, Azure CLI, environment variables, all - // tried by the Azdls service's own default credential chain), which this - // must not silently break for callers who never asked for anonymous - // access. Whenever `allow_anonymous` is false, this always goes through - // the real, signed Azdls service exactly as before. - if allow_anonymous { - return azdls_anonymous_operator_build(config, path); - } - - let mut builder = config.clone().into_builder(); - - if config.endpoint.is_none() { - // If no endpoint is provided, we construct it from the fully-qualified path. - builder = builder.endpoint(&path.as_endpoint()); - } - builder = builder.filesystem(&path.filesystem); - - Ok(opendal::Operator::new(builder)?) -} - -/// Builds a read-only, unauthenticated `opendal::Operator` (via opendal's -/// generic `Http` service) against the same account/container endpoint the -/// signed Azdls builder would have used. Only `read` and `stat` are -/// supported, which matches Iceberg's own read-only file access pattern. -fn azdls_anonymous_operator_build( - config: &AzdlsConfig, - path: &AzureStoragePath, -) -> Result { - let endpoint = config - .endpoint - .clone() - .unwrap_or_else(|| path.as_endpoint()); - let mut http_config = opendal::services::HttpConfig::default(); - http_config.endpoint = Some(endpoint); - http_config.root = Some(format!("/{}", path.filesystem)); - // Deliberately leave username/password/token unset: opendal's Http - // service only adds an Authorization header when one of these is set, so - // leaving them unset is what makes this a genuinely unsigned request. - Ok(opendal::Operator::new(http_config.into_builder())?) -} - -/// Represents a fully qualified path to blob/ file in Azure Storage. -#[derive(Debug, PartialEq)] -struct AzureStoragePath { - /// The scheme of the URL, e.g., `abfss`, `abfs`, `wasbs`, or `wasb`. - scheme: AzureStorageScheme, - - /// Under Blob Storage, this is considered the _container_. - filesystem: String, - - account_name: String, - - /// The endpoint suffix, e.g., `core.windows.net` for the public cloud - /// endpoint. - endpoint_suffix: String, - - /// Path to the file. - /// - /// It is relative to the `root` of the `AzdlsConfig`. - path: String, -} - -impl AzureStoragePath { - /// Converts the AzureStoragePath into a full endpoint URL. - /// - /// This is possible because the path is fully qualified. - fn as_endpoint(&self) -> String { - format!( - "{}://{}.dfs.{}", - self.scheme.as_http_scheme(), - self.account_name, - self.endpoint_suffix - ) - } -} - -impl FromStr for AzureStoragePath { - type Err = Error; - - fn from_str(path: &str) -> Result { - let url = Url::parse(path)?; - - let filesystem = url.username(); - ensure_data_valid!( - !filesystem.is_empty(), - "AzureStoragePath: No container or filesystem name in path: {}", - path - ); - - let (account_name, storage_service, endpoint_suffix) = parse_azure_storage_endpoint(&url)?; - let scheme = validate_storage_and_scheme(storage_service, url.scheme())?; - - Ok(AzureStoragePath { - scheme, - filesystem: filesystem.to_string(), - account_name: account_name.to_string(), - endpoint_suffix: endpoint_suffix.to_string(), - path: url.path().to_string(), - }) - } -} - -fn parse_azure_storage_endpoint(url: &Url) -> Result<(&str, &str, &str)> { - let host = url.host_str().ok_or(Error::new( - ErrorKind::DataInvalid, - "AzureStoragePath: No host", - ))?; - - let (account_name, endpoint) = host.split_once('.').ok_or(Error::new( - ErrorKind::DataInvalid, - "AzureStoragePath: No account name", - ))?; - if account_name.is_empty() { - return Err(Error::new( - ErrorKind::DataInvalid, - "AzureStoragePath: No account name", - )); - } - - let (storage, endpoint_suffix) = endpoint.split_once('.').ok_or(Error::new( - ErrorKind::DataInvalid, - "AzureStoragePath: No storage service", - ))?; - - Ok((account_name, storage, endpoint_suffix)) -} - -fn validate_storage_and_scheme( - storage_service: &str, - scheme_str: &str, -) -> Result { - let scheme = scheme_str.parse::()?; - // Azure actually is oblivious to what we use for the scheme here. - // It actually supports both dfs and blob endpoints for all storage kinds. - // We should route those to different OpenDAL operators, but given that we don't - // do that today but map both schemes/endpoints to the same ADLS OpenDAL operator - // (which uses dfs endpoint), we might as well accept wasb URL for dfs endpoint, - // and abfs URL for blob endpoint. Especially since some implementations (e.g. Snowflake) - // always use abfs in URL, regardless of the endpoint. - ensure_data_valid!( - storage_service == "dfs" || storage_service == "blob", - "AzureStoragePath: Unexpected storage service for abfs[s]: {}", - storage_service - ); - Ok(scheme) -} - -#[cfg(test)] -mod tests { - use std::collections::HashMap; - - use opendal::services::AzdlsConfig; - - use super::{AzureStoragePath, AzureStorageScheme, azdls_config_parse, azdls_create_operator}; - use crate::io::storage::config::ADLS_SAS_TOKEN; - - #[test] - fn test_azdls_config_parse() { - let test_cases = vec![ - ( - "account name and key", - HashMap::from([ - (super::ADLS_ACCOUNT_NAME.to_string(), "test".to_string()), - (super::ADLS_ACCOUNT_KEY.to_string(), "secret".to_string()), - ]), - Some(AzdlsConfig { - account_name: Some("test".to_string()), - account_key: Some("secret".to_string()), - ..Default::default() - }), - ), - ( - "endpoint pointing to azurite", - HashMap::from([ - ( - super::ADLS_ENDPOINT.to_string(), - "http://127.0.0.1:10000/devstoreaccount1".to_string(), - ), - ( - super::ADLS_ACCOUNT_NAME.to_string(), - "devstoreaccount1".to_string(), - ), - (super::ADLS_ACCOUNT_KEY.to_string(), "secret".to_string()), - ]), - Some(AzdlsConfig { - endpoint: Some("http://127.0.0.1:10000/devstoreaccount1".to_string()), - account_name: Some("devstoreaccount1".to_string()), - account_key: Some("secret".to_string()), - ..Default::default() - }), - ), - ( - "account name and SAS token", - HashMap::from([ - (super::ADLS_ACCOUNT_NAME.to_string(), "test".to_string()), - (ADLS_SAS_TOKEN.to_string(), "token".to_string()), - ]), - Some(AzdlsConfig { - account_name: Some("test".to_string()), - sas_token: Some("token".to_string()), - ..Default::default() - }), - ), - ( - "account name and ADD credentials", - HashMap::from([ - (super::ADLS_ACCOUNT_NAME.to_string(), "test".to_string()), - (super::ADLS_CLIENT_ID.to_string(), "abcdef".to_string()), - (super::ADLS_CLIENT_SECRET.to_string(), "secret".to_string()), - (super::ADLS_TENANT_ID.to_string(), "12345".to_string()), - ]), - Some(AzdlsConfig { - account_name: Some("test".to_string()), - client_id: Some("abcdef".to_string()), - client_secret: Some("secret".to_string()), - tenant_id: Some("12345".to_string()), - ..Default::default() - }), - ), - ( - "account-specific SAS token with full domain", - HashMap::from([ - ( - super::ADLS_ACCOUNT_NAME.to_string(), - "azteststorage".to_string(), - ), - ( - "adls.sas-token.azteststorage.blob.core.windows.net".to_string(), - "token-full".to_string(), - ), - ( - "adls.sas-token.azteststorage".to_string(), - "token-account".to_string(), - ), - ]), - Some(AzdlsConfig { - account_name: Some("azteststorage".to_string()), - sas_token: Some("token-account".to_string()), // Should pick the shorter one - ..Default::default() - }), - ), - ( - "account-specific SAS token with only full domain", - HashMap::from([ - ( - super::ADLS_ACCOUNT_NAME.to_string(), - "myaccount".to_string(), - ), - ( - "adls.sas-token.myaccount.blob.core.windows.net".to_string(), - "token-specific".to_string(), - ), - ]), - Some(AzdlsConfig { - account_name: Some("myaccount".to_string()), - sas_token: Some("token-specific".to_string()), - ..Default::default() - }), - ), - ( - "SAS token without account name picks shortest", - HashMap::from([ - (ADLS_SAS_TOKEN.to_string(), "token-generic".to_string()), - ( - "adls.sas-token.someaccount".to_string(), - "token-account".to_string(), - ), - ( - "adls.sas-token.someaccount.blob.core.windows.net".to_string(), - "token-specific".to_string(), - ), - ]), - Some(AzdlsConfig { - sas_token: Some("token-generic".to_string()), // Should pick the shortest one - ..Default::default() - }), - ), - ]; - - for (name, properties, expected) in test_cases { - let config = azdls_config_parse(properties); - match expected { - Some(expected_config) => { - assert!(config.is_ok(), "Test case {name} failed: {config:?}"); - assert_eq!(config.unwrap(), expected_config, "Test case: {name}"); - } - None => { - assert!(config.is_err(), "Test case {name} expected error."); - } - } - } - } - - #[test] - fn test_azdls_create_operator() { - let test_cases = vec![ - ( - "basic", - ( - "abfss://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - account_name: Some("myaccount".to_string()), - endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()), - ..Default::default() - }, - AzureStorageScheme::Abfss, - ), - Some(("myfs", "/path/to/file.parquet")), - ), - ( - "different account", - ( - "abfss://myfs@anotheraccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - account_name: Some("myaccount".to_string()), - endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()), - ..Default::default() - }, - AzureStorageScheme::Abfss, - ), - None, - ), - ( - "different scheme", - ( - "wasbs://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - account_name: Some("myaccount".to_string()), - endpoint: Some("https://myaccount.dfs.core.windows.net".to_string()), - ..Default::default() - }, - AzureStorageScheme::Abfss, - ), - None, - ), - ( - "incompatible scheme for endpoint", - ( - "abfs://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - account_name: Some("myaccount".to_string()), - endpoint: Some("http://myaccount.dfs.core.windows.net".to_string()), - ..Default::default() - }, - AzureStorageScheme::Abfss, - ), - None, - ), - ( - "different endpoint suffix", - ( - "abfss://somefs@myaccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - account_name: Some("myaccount".to_string()), - endpoint: Some("https://myaccount.dfs.core.chinacloudapi.cn".to_string()), - ..Default::default() - }, - AzureStorageScheme::Abfss, - ), - None, - ), - ( - "endpoint inferred from fully qualified path", - ( - "abfs://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet", - AzdlsConfig { - filesystem: "myfs".to_string(), - account_name: Some("myaccount".to_string()), - endpoint: None, - ..Default::default() - }, - AzureStorageScheme::Abfs, - ), - Some(("myfs", "/path/to/file.parquet")), - ), - ( - "azurite endpoint with explicit configuration", - ( - "wasb://testfs@devstoreaccount1.blob.core.windows.net/path/to/data.parquet", - AzdlsConfig { - account_name: Some("devstoreaccount1".to_string()), - endpoint: Some("http://127.0.0.1:10000/devstoreaccount1".to_string()), - account_key: Some("secret".to_string()), - ..Default::default() - }, - AzureStorageScheme::Wasb, - ), - Some(("testfs", "/path/to/data.parquet")), - ), - ]; - - for (name, input, expected) in test_cases { - let result = azdls_create_operator(input.0, &input.1, &input.2, false); - match expected { - Some((expected_filesystem, expected_path)) => { - assert!(result.is_ok(), "Test case {name} failed: {result:?}"); - - let (op, relative_path) = result.unwrap(); - assert_eq!(op.info().name(), expected_filesystem); - assert_eq!(relative_path, expected_path); - } - None => { - assert!(result.is_err(), "Test case {name} expected error."); - } - } - } - } - - #[test] - fn test_azdls_create_operator_anonymous() { - let path = "abfss://myfs@myaccount.dfs.core.windows.net/path/to/file.parquet"; - let config = AzdlsConfig::default(); - - // No credentials, no `allow_anonymous`: goes through the real (signed) - // Azdls service as before -- unaffected by this fix. Building the - // operator itself still succeeds (loading a credential is deferred - // until an actual request is signed), so this only demonstrates it's - // *not* the anonymous HTTP fallback, via the scheme name. - let (op, relative_path) = - azdls_create_operator(path, &config, &AzureStorageScheme::Abfss, false) - .expect("operator should build without a credential"); - assert_eq!(relative_path, "/path/to/file.parquet"); - assert_eq!(op.info().scheme(), "azdls"); - - // No credentials, `allow_anonymous=true`: routes to the anonymous - // HTTP fallback instead. - let (op, relative_path) = - azdls_create_operator(path, &config, &AzureStorageScheme::Abfss, true) - .expect("anonymous operator should build"); - assert_eq!(relative_path, "/path/to/file.parquet"); - assert_eq!(op.info().scheme(), "http"); - - // `allow_anonymous=true` takes priority even when a credential is - // also configured -- matching S3's `skip_signature`, which is checked - // unconditionally before any credential is loaded. Callers only ever - // set this alongside a real credential by mistake; since it's an - // explicit opt-in flag (never inferred), that mistake is on them. - let config_with_key = AzdlsConfig { - account_key: Some("secret".to_string()), - ..Default::default() - }; - let (op, _) = - azdls_create_operator(path, &config_with_key, &AzureStorageScheme::Abfss, true) - .expect("anonymous operator should still build"); - assert_eq!(op.info().scheme(), "http"); - } - - #[test] - fn test_azure_storage_path_parse() { - let test_cases = vec![ - ( - "succeeds", - "abfss://somefs@myaccount.dfs.core.windows.net/path/to/file.parquet", - Some(AzureStoragePath { - scheme: AzureStorageScheme::Abfss, - filesystem: "somefs".to_string(), - account_name: "myaccount".to_string(), - endpoint_suffix: "core.windows.net".to_string(), - path: "/path/to/file.parquet".to_string(), - }), - ), - ( - "unexpected scheme", - "adls://somefs@myaccount.dfs.core.windows.net/path/to/file.parquet", - None, - ), - ( - "no filesystem", - "abfss://myaccount.dfs.core.windows.net/path/to/file.parquet", - None, - ), - ( - "no account name", - "abfs://myfs@dfs.core.windows.net/path/to/file.parquet", - None, - ), - ]; - - for (name, input, expected) in test_cases { - let result = input.parse::(); - match expected { - Some(expected_path) => { - assert!(result.is_ok(), "Test case {name} failed: {result:?}"); - assert_eq!(result.unwrap(), expected_path, "Test case: {name}"); - } - None => { - assert!(result.is_err(), "Test case {name} expected error."); - } - } - } - } - - #[test] - fn test_azure_storage_path_endpoint() { - let test_cases = vec![ - ( - "abfss uses https", - AzureStoragePath { - scheme: AzureStorageScheme::Abfss, - filesystem: "myfs".to_string(), - account_name: "myaccount".to_string(), - endpoint_suffix: "core.windows.net".to_string(), - path: "/path/to/file.parquet".to_string(), - }, - "https://myaccount.dfs.core.windows.net", - ), - ( - "abfs uses http", - AzureStoragePath { - scheme: AzureStorageScheme::Abfs, - filesystem: "myfs".to_string(), - account_name: "myaccount".to_string(), - endpoint_suffix: "core.windows.net".to_string(), - path: "/path/to/file.parquet".to_string(), - }, - "http://myaccount.dfs.core.windows.net", - ), - ( - "wasbs uses https and dfs", - AzureStoragePath { - scheme: AzureStorageScheme::Abfss, - filesystem: "myfs".to_string(), - account_name: "myaccount".to_string(), - endpoint_suffix: "core.windows.net".to_string(), - path: "/path/to/file.parquet".to_string(), - }, - "https://myaccount.dfs.core.windows.net", - ), - ]; - - for (name, path, expected) in test_cases { - let endpoint = path.as_endpoint(); - assert_eq!(endpoint, expected, "Test case: {name}"); - } - } -} diff --git a/crates/iceberg/src/io/storage/opendal/fs.rs b/crates/iceberg/src/io/storage/opendal/fs.rs deleted file mode 100644 index 71216b73d7..0000000000 --- a/crates/iceberg/src/io/storage/opendal/fs.rs +++ /dev/null @@ -1,29 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use opendal::Operator; -use opendal::services::FsConfig; - -use crate::Result; - -/// Build new opendal operator from give path. -pub(crate) fn fs_config_build() -> Result { - let mut cfg = FsConfig::default(); - cfg.root = Some("/".to_string()); - - Ok(Operator::from_config(cfg)?) -} diff --git a/crates/iceberg/src/io/storage/opendal/gcs.rs b/crates/iceberg/src/io/storage/opendal/gcs.rs deleted file mode 100644 index 5efcfc6099..0000000000 --- a/crates/iceberg/src/io/storage/opendal/gcs.rs +++ /dev/null @@ -1,86 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. -//! Google Cloud Storage properties - -use std::collections::HashMap; - -use opendal::Operator; -use opendal::services::GcsConfig; -use url::Url; - -use super::super::config::{ - GCS_ALLOW_ANONYMOUS, GCS_CREDENTIALS_JSON, GCS_DISABLE_CONFIG_LOAD, GCS_DISABLE_VM_METADATA, - GCS_NO_AUTH, GCS_SERVICE_HOST, GCS_TOKEN, -}; -use crate::io::is_truthy; -use crate::{Error, ErrorKind, Result}; - -/// Parse iceberg properties to [`GcsConfig`]. -pub(crate) fn gcs_config_parse(mut m: HashMap) -> Result { - let mut cfg = GcsConfig::default(); - - if let Some(cred) = m.remove(GCS_CREDENTIALS_JSON) { - cfg.credential = Some(cred); - } - - if let Some(token) = m.remove(GCS_TOKEN) { - cfg.token = Some(token); - } - - if let Some(endpoint) = m.remove(GCS_SERVICE_HOST) { - cfg.endpoint = Some(endpoint); - } - - if m.remove(GCS_NO_AUTH).is_some() { - cfg.skip_signature = true; - cfg.disable_vm_metadata = true; - cfg.disable_config_load = true; - } - - if let Some(allow_anonymous) = m.remove(GCS_ALLOW_ANONYMOUS) - && is_truthy(allow_anonymous.to_lowercase().as_str()) - { - cfg.skip_signature = true; - } - if let Some(disable_ec2_metadata) = m.remove(GCS_DISABLE_VM_METADATA) - && is_truthy(disable_ec2_metadata.to_lowercase().as_str()) - { - cfg.disable_vm_metadata = true; - }; - if let Some(disable_config_load) = m.remove(GCS_DISABLE_CONFIG_LOAD) - && is_truthy(disable_config_load.to_lowercase().as_str()) - { - cfg.disable_config_load = true; - }; - - Ok(cfg) -} - -/// Build a new OpenDAL [`Operator`] based on a provided [`GcsConfig`]. -pub(crate) fn gcs_config_build(cfg: &GcsConfig, path: &str) -> Result { - let url = Url::parse(path)?; - let bucket = url.host_str().ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Invalid gcs url: {path}, bucket is required"), - ) - })?; - - let mut cfg = cfg.clone(); - cfg.bucket = bucket.to_string(); - Ok(Operator::from_config(cfg)?) -} diff --git a/crates/iceberg/src/io/storage/opendal/memory.rs b/crates/iceberg/src/io/storage/opendal/memory.rs deleted file mode 100644 index 142cd4b311..0000000000 --- a/crates/iceberg/src/io/storage/opendal/memory.rs +++ /dev/null @@ -1,25 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use opendal::Operator; -use opendal::services::MemoryConfig; - -use crate::Result; - -pub(crate) fn memory_config_build() -> Result { - Ok(Operator::from_config(MemoryConfig::default())?) -} diff --git a/crates/iceberg/src/io/storage/opendal/mod.rs b/crates/iceberg/src/io/storage/opendal/mod.rs deleted file mode 100644 index dd9f79e529..0000000000 --- a/crates/iceberg/src/io/storage/opendal/mod.rs +++ /dev/null @@ -1,754 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -//! OpenDAL-based storage implementation. - -use std::collections::HashMap; -use std::ops::Range; -use std::sync::Arc; - -use async_trait::async_trait; -#[cfg(feature = "storage-azdls")] -use azdls::AzureStorageScheme; -use bytes::Bytes; -use futures::StreamExt; -use futures::stream::BoxStream; -use opendal::Operator; -use opendal::layers::RetryLayer; -#[cfg(feature = "storage-azdls")] -use opendal::services::AzdlsConfig; -#[cfg(feature = "storage-gcs")] -use opendal::services::GcsConfig; -#[cfg(feature = "storage-oss")] -use opendal::services::OssConfig; -#[cfg(feature = "storage-s3")] -use opendal::services::S3Config; -#[cfg(feature = "storage-s3")] -pub use s3::CustomAwsCredentialLoader; -use serde::{Deserialize, Serialize}; -use url::Url; - -use crate::catalog::{NamespaceIdent, TableIdent}; -use crate::io::refreshable_storage::RefreshableOpenDalStorageBuilder; -#[cfg(feature = "storage-azdls")] -use crate::io::storage::config::ADLS_ALLOW_ANONYMOUS; -use crate::io::storage::config::{PROP_METADATA_LOCATION, PROP_TABLE_IDENT}; -use crate::io::{ - FileMetadata, FileRead, FileWrite, InputFile, OutputFile, Storage, StorageConfig, - StorageCredentialsLoader, StorageFactory, -}; -use crate::{Error, ErrorKind, Result}; - -#[cfg(feature = "storage-azdls")] -mod azdls; -#[cfg(feature = "storage-fs")] -mod fs; -#[cfg(feature = "storage-gcs")] -mod gcs; -#[cfg(feature = "storage-memory")] -mod memory; -#[cfg(feature = "storage-oss")] -mod oss; -#[cfg(feature = "storage-s3")] -mod s3; - -#[cfg(feature = "storage-azdls")] -use azdls::*; -#[cfg(feature = "storage-fs")] -use fs::*; -#[cfg(feature = "storage-gcs")] -use gcs::*; -#[cfg(feature = "storage-memory")] -use memory::*; -#[cfg(feature = "storage-oss")] -use oss::*; -#[cfg(feature = "storage-s3")] -pub use s3::*; - -/// OpenDAL-based storage factory. -/// -/// Maps scheme to the corresponding OpenDalStorage storage variant. -/// Use this factory with `FileIOBuilder::new(factory)` to create FileIO instances. -#[derive(Clone, Debug, Serialize, Deserialize)] -pub enum OpenDalStorageFactory { - /// Memory storage factory. - #[cfg(feature = "storage-memory")] - Memory, - /// Local filesystem storage factory. - #[cfg(feature = "storage-fs")] - Fs, - /// S3 storage factory. - #[cfg(feature = "storage-s3")] - S3 { - /// s3 storage could have `s3://` and `s3a://`. - /// Storing the scheme string here to return the correct path. - configured_scheme: String, - /// Custom AWS credential loader. - #[serde(skip)] - customized_credential_load: Option, - }, - /// GCS storage factory. - #[cfg(feature = "storage-gcs")] - Gcs, - /// OSS storage factory. - #[cfg(feature = "storage-oss")] - Oss, - /// Azure Data Lake Storage factory. - #[cfg(feature = "storage-azdls")] - Azdls { - /// The configured Azure storage scheme. - configured_scheme: AzureStorageScheme, - }, -} - -#[cfg(feature = "storage-azdls")] -fn azdls_allow_anonymous(properties: &HashMap) -> bool { - properties - .get(ADLS_ALLOW_ANONYMOUS) - .is_some_and(|v| crate::io::is_truthy(v.to_lowercase().as_str())) -} - -#[typetag::serde(name = "OpenDalStorageFactory")] -impl StorageFactory for OpenDalStorageFactory { - #[allow(unused_variables)] - fn build(&self, config: &StorageConfig) -> Result> { - match self { - #[cfg(feature = "storage-memory")] - OpenDalStorageFactory::Memory => { - Ok(Arc::new(OpenDalStorage::Memory(memory_config_build()?))) - } - #[cfg(feature = "storage-fs")] - OpenDalStorageFactory::Fs => Ok(Arc::new(OpenDalStorage::LocalFs)), - #[cfg(feature = "storage-s3")] - OpenDalStorageFactory::S3 { - configured_scheme, - customized_credential_load, - } => Ok(Arc::new(OpenDalStorage::S3 { - configured_scheme: configured_scheme.clone(), - config: s3_config_parse(config.props().clone())?.into(), - customized_credential_load: customized_credential_load.clone(), - })), - #[cfg(feature = "storage-gcs")] - OpenDalStorageFactory::Gcs => Ok(Arc::new(OpenDalStorage::Gcs { - config: gcs_config_parse(config.props().clone())?.into(), - })), - #[cfg(feature = "storage-oss")] - OpenDalStorageFactory::Oss => Ok(Arc::new(OpenDalStorage::Oss { - config: oss_config_parse(config.props().clone())?.into(), - })), - #[cfg(feature = "storage-azdls")] - OpenDalStorageFactory::Azdls { configured_scheme } => { - Ok(Arc::new(OpenDalStorage::Azdls { - configured_scheme: configured_scheme.clone(), - allow_anonymous: azdls_allow_anonymous(config.props()), - config: azdls_config_parse(config.props().clone())?.into(), - })) - } - #[cfg(all( - not(feature = "storage-memory"), - not(feature = "storage-fs"), - not(feature = "storage-s3"), - not(feature = "storage-gcs"), - not(feature = "storage-oss"), - not(feature = "storage-azdls"), - ))] - _ => Err(Error::new( - ErrorKind::FeatureUnsupported, - "No storage service has been enabled", - )), - } - } -} - -/// Default memory operator for serde deserialization. -#[cfg(feature = "storage-memory")] -fn default_memory_operator() -> Operator { - memory_config_build().expect("Failed to create default memory operator") -} - -/// OpenDAL-based storage implementation. -#[derive(Clone, Debug, Serialize, Deserialize)] -pub enum OpenDalStorage { - /// Memory storage variant. - #[cfg(feature = "storage-memory")] - Memory(#[serde(skip, default = "self::default_memory_operator")] Operator), - /// Local filesystem storage variant. - #[cfg(feature = "storage-fs")] - LocalFs, - /// S3 storage variant. - #[cfg(feature = "storage-s3")] - S3 { - /// s3 storage could have `s3://` and `s3a://`. - /// Storing the scheme string here to return the correct path. - configured_scheme: String, - /// S3 configuration. - config: Arc, - /// Custom AWS credential loader. - #[serde(skip)] - customized_credential_load: Option, - }, - /// GCS storage variant. - #[cfg(feature = "storage-gcs")] - Gcs { - /// GCS configuration. - config: Arc, - }, - /// OSS storage variant. - #[cfg(feature = "storage-oss")] - Oss { - /// OSS configuration. - config: Arc, - }, - /// Azure Data Lake Storage variant. - /// Expects paths of the form - /// `abfs[s]://@.dfs./` or - /// `wasb[s]://@.blob./`. - #[cfg(feature = "storage-azdls")] - #[allow(private_interfaces)] - Azdls { - /// The configured Azure storage scheme. - /// Because Azdls accepts multiple possible schemes, we store the full - /// passed scheme here to later validate schemes passed via paths. - configured_scheme: AzureStorageScheme, - /// Azure DLS configuration. - config: Arc, - /// Whether to fall back to genuinely anonymous, unsigned access when - /// no credential is configured. See [`ADLS_ALLOW_ANONYMOUS`]. - allow_anonymous: bool, - }, - /// Wraps any storage with credential refresh capability. - Refreshable { - /// The refreshable storage backend. - /// `None` only after deserialization (cannot be reconstructed from serialized form). - #[serde(skip)] - backend: Option>, - }, -} - -impl OpenDalStorage { - /// Build storage from scheme and properties. - /// - /// Used by `RefreshableOpenDalStorage` to create and rebuild its inner storage on credential refresh. - /// Note: for S3, `customized_credential_load` is always `None` here; the refreshable wrapper - /// handles all credential rotation via `StorageCredentialsLoader`. - #[allow(unused_variables)] - pub(crate) fn build_from_props( - scheme_str: &str, - props: HashMap, - ) -> Result { - match scheme_str { - #[cfg(feature = "storage-memory")] - "memory" => Ok(Self::Memory(memory_config_build()?)), - #[cfg(feature = "storage-fs")] - "file" | "" => Ok(Self::LocalFs), - #[cfg(feature = "storage-s3")] - "s3" | "s3a" => Ok(Self::S3 { - configured_scheme: scheme_str.to_string(), - config: s3_config_parse(props)?.into(), - customized_credential_load: None, - }), - #[cfg(feature = "storage-gcs")] - "gs" | "gcs" => Ok(Self::Gcs { - config: gcs_config_parse(props)?.into(), - }), - #[cfg(feature = "storage-oss")] - "oss" => Ok(Self::Oss { - config: oss_config_parse(props)?.into(), - }), - #[cfg(feature = "storage-azdls")] - "abfss" | "abfs" | "wasbs" | "wasb" => { - let configured_scheme = scheme_str.parse::()?; - let allow_anonymous = azdls_allow_anonymous(&props); - Ok(Self::Azdls { - config: azdls_config_parse(props)?.into(), - configured_scheme, - allow_anonymous, - }) - } - _ => Err(Error::new( - ErrorKind::FeatureUnsupported, - format!("Constructing file io from scheme: {scheme_str} not supported now"), - )), - } - } - - /// Creates operator from path. - /// - /// # Arguments - /// - /// * path: It should be *absolute* path starting with scheme string used to construct [`FileIO`]. - /// - /// # Returns - /// - /// The return value consists of two parts: - /// - /// * An [`opendal::Operator`] instance used to operate on file. - /// * Relative path to the root uri of [`opendal::Operator`]. - #[allow(unreachable_code, unused_variables, unreachable_patterns)] - pub(crate) fn create_operator<'a>( - &self, - path: &'a impl AsRef, - ) -> Result<(Operator, &'a str)> { - let path = path.as_ref(); - let (operator, relative_path): (Operator, &str) = match self { - #[cfg(feature = "storage-memory")] - OpenDalStorage::Memory(op) => { - if let Some(stripped) = path.strip_prefix("memory:/") { - (op.clone(), stripped) - } else { - (op.clone(), &path[1..]) - } - } - #[cfg(feature = "storage-fs")] - OpenDalStorage::LocalFs => { - let op = fs_config_build()?; - if let Some(stripped) = path.strip_prefix("file:/") { - (op, stripped) - } else { - (op, &path[1..]) - } - } - #[cfg(feature = "storage-s3")] - OpenDalStorage::S3 { - configured_scheme, - config, - customized_credential_load, - } => { - let op = s3_config_build(config, customized_credential_load, path)?; - let op_info = op.info(); - - // Check prefix of s3 path. - let prefix = format!("{}://{}/", configured_scheme, op_info.name()); - if path.starts_with(&prefix) { - (op, &path[prefix.len()..]) - } else { - return Err(Error::new( - ErrorKind::DataInvalid, - format!("Invalid s3 url: {path}, should start with {prefix}"), - )); - } - } - #[cfg(feature = "storage-gcs")] - OpenDalStorage::Gcs { config } => { - let operator = gcs_config_build(config, path)?; - let prefix = format!("gs://{}/", operator.info().name()); - if path.starts_with(&prefix) { - (operator, &path[prefix.len()..]) - } else { - return Err(Error::new( - ErrorKind::DataInvalid, - format!("Invalid gcs url: {path}, should start with {prefix}"), - )); - } - } - #[cfg(feature = "storage-oss")] - OpenDalStorage::Oss { config } => { - let op = oss_config_build(config, path)?; - let prefix = format!("oss://{}/", op.info().name()); - if path.starts_with(&prefix) { - (op, &path[prefix.len()..]) - } else { - return Err(Error::new( - ErrorKind::DataInvalid, - format!("Invalid oss url: {path}, should start with {prefix}"), - )); - } - } - #[cfg(feature = "storage-azdls")] - OpenDalStorage::Azdls { - configured_scheme, - config, - allow_anonymous, - } => azdls_create_operator(path, config, configured_scheme, *allow_anonymous)?, - OpenDalStorage::Refreshable { backend } => { - let backend = backend.as_ref().ok_or_else(|| { - Error::new( - ErrorKind::Unexpected, - "Deserialized RefreshableOpenDalStorage cannot create operators", - ) - })?; - let (operator, relative_path) = backend.refreshable_create_operator(path)?; - // relative_path is always a suffix of `path`, so slice the - // input directly to avoid an allocation. - let relative_path_ref = &path[path.len() - relative_path.len()..]; - (operator, relative_path_ref) - } - #[cfg(all( - not(feature = "storage-s3"), - not(feature = "storage-fs"), - not(feature = "storage-gcs"), - not(feature = "storage-oss"), - not(feature = "storage-azdls"), - ))] - _ => { - return Err(Error::new( - ErrorKind::FeatureUnsupported, - "No storage service has been enabled", - )); - } - }; - - // Transient errors are common for object stores; however there's no - // harm in retrying temporary failures for other storage backends as well. - // The Refreshable variant already has a RetryLayer from the inner - // create_operator call, so skip adding a second one to avoid - // unnecessary credential refreshes on transient errors. - let operator = match self { - OpenDalStorage::Refreshable { .. } => operator, - _ => operator.layer(RetryLayer::new()), - }; - Ok((operator, relative_path)) - } -} - -#[typetag::serde(name = "OpenDalStorage")] -#[async_trait] -impl Storage for OpenDalStorage { - async fn exists(&self, path: &str) -> Result { - let (op, relative_path) = self.create_operator(&path)?; - Ok(op.exists(relative_path).await?) - } - - async fn metadata(&self, path: &str) -> Result { - let (op, relative_path) = self.create_operator(&path)?; - let meta = op.stat(relative_path).await?; - Ok(FileMetadata { - size: meta.content_length(), - }) - } - - async fn read(&self, path: &str) -> Result { - let (op, relative_path) = self.create_operator(&path)?; - Ok(op.read(relative_path).await?.to_bytes()) - } - - async fn reader(&self, path: &str) -> Result> { - let (op, relative_path) = self.create_operator(&path)?; - Ok(Box::new(op.reader(relative_path).await?)) - } - - async fn write(&self, path: &str, bs: Bytes) -> Result<()> { - let (op, relative_path) = self.create_operator(&path)?; - op.write(relative_path, bs).await?; - Ok(()) - } - - async fn writer(&self, path: &str) -> Result> { - let (op, relative_path) = self.create_operator(&path)?; - Ok(Box::new(op.writer(relative_path).await?)) - } - - async fn delete(&self, path: &str) -> Result<()> { - let (op, relative_path) = self.create_operator(&path)?; - Ok(op.delete(relative_path).await?) - } - - async fn delete_stream(&self, mut paths: BoxStream<'static, String>) -> Result<()> { - while let Some(path) = paths.next().await { - self.delete(&path).await?; - } - Ok(()) - } - - async fn delete_prefix(&self, path: &str) -> Result<()> { - let (op, relative_path) = self.create_operator(&path)?; - let path = if relative_path.ends_with('/') { - relative_path.to_string() - } else { - format!("{relative_path}/") - }; - Ok(op.delete_with(&path).recursive(true).await?) - } - - #[allow(unreachable_code, unused_variables)] - fn new_input(&self, path: &str) -> Result { - Ok(InputFile::new(Arc::new(self.clone()), path.to_string())) - } - - #[allow(unreachable_code, unused_variables)] - fn new_output(&self, path: &str) -> Result { - Ok(OutputFile::new(Arc::new(self.clone()), path.to_string())) - } -} - -// OpenDAL implementations for FileRead and FileWrite traits - -#[async_trait] -impl FileRead for opendal::Reader { - async fn read(&self, range: Range) -> Result { - Ok(opendal::Reader::read(self, range).await?.to_bytes()) - } -} - -#[async_trait] -impl FileWrite for opendal::Writer { - async fn write(&mut self, bs: Bytes) -> Result<()> { - Ok(opendal::Writer::write(self, bs).await?) - } - - async fn close(&mut self) -> Result<()> { - let _ = opendal::Writer::close(self).await?; - Ok(()) - } -} - -/// A [`StorageFactory`] that creates a [`OpenDalStorage::Refreshable`] backend with -/// automatic credential rotation. -/// -/// Inject it at catalog construction time via `with_storage_factory`. At table-load time -/// the catalog populates [`StorageConfig`] with the table identity and metadata location; -/// `build()` reads those to pass context to the credential loader on each refresh. -/// -/// # Example -/// -/// ```rust,no_run -/// use std::sync::Arc; -/// -/// use iceberg::io::{RefreshableStorageFactory, StorageCredentialsLoader}; -/// -/// // Implement your own loader: -/// // let loader: Arc = ...; -/// // let factory = Arc::new(RefreshableStorageFactory::new(loader)); -/// // catalog_config.with_storage_factory(factory); -/// ``` -#[derive(Debug, Serialize, Deserialize)] -pub struct RefreshableStorageFactory { - /// The credentials loader. `None` only after serde deserialization (field is skipped). - #[serde(skip)] - credentials_loader: Option>, -} - -impl RefreshableStorageFactory { - /// Creates a new factory. - pub fn new(credentials_loader: Arc) -> Self { - Self { - credentials_loader: Some(credentials_loader), - } - } -} - -#[typetag::serde] -impl StorageFactory for RefreshableStorageFactory { - fn build(&self, config: &StorageConfig) -> Result> { - let loader = self.credentials_loader.as_ref().ok_or_else(|| { - Error::new( - ErrorKind::Unexpected, - "RefreshableStorageFactory: credentials loader unavailable after deserialization", - ) - })?; - - // Extract runtime context from props, stripping the internal keys so they - // don't leak into the underlying OpenDAL operator configuration. - let mut props = config.props().clone(); - let location = props.remove(PROP_METADATA_LOCATION).unwrap_or_default(); - let scheme = Url::parse(&location) - .map(|u| u.scheme().to_string()) - .unwrap_or_default(); - let table_ident = props - .remove(PROP_TABLE_IDENT) - .and_then(|s| serde_json::from_str::(&s).ok()) - .unwrap_or_else(|| { - TableIdent::new( - NamespaceIdent::new("unknown".to_string()), - "unknown".to_string(), - ) - }); - - let backend = RefreshableOpenDalStorageBuilder::new() - .scheme(scheme) - .base_props(props) - .credentials_loader(Arc::clone(loader)) - .location(location) - .table_ident(table_ident) - .build()?; - Ok(Arc::new(OpenDalStorage::Refreshable { - backend: Some(backend), - })) - } -} - -/// A [`StorageFactory`] that routes to the appropriate [`OpenDalStorageFactory`] variant -/// based on the URI scheme parsed from the metadata location property. -/// -/// Unlike [`OpenDalStorageFactory`] (which is pre-configured for a specific scheme), -/// this factory determines the scheme at build time from the metadata location. This is -/// useful when a catalog serves tables across multiple storage backends (e.g. S3 and GCS). -/// -/// # Example -/// -/// ```rust,no_run -/// use std::sync::Arc; -/// -/// use iceberg::io::OpenDalRoutingStorageFactory; -/// -/// let factory = Arc::new(OpenDalRoutingStorageFactory); -/// // Pass to catalog builder via .with_storage_factory(factory) -/// ``` -#[derive(Debug, Serialize, Deserialize)] -pub struct OpenDalRoutingStorageFactory; - -#[typetag::serde] -impl StorageFactory for OpenDalRoutingStorageFactory { - fn build(&self, config: &StorageConfig) -> Result> { - let mut props = config.props().clone(); - let location = props.remove(PROP_METADATA_LOCATION).ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - "OpenDalRoutingStorageFactory: missing metadata location in config props", - ) - })?; - let scheme = Url::parse(&location) - .map(|u| u.scheme().to_string()) - .unwrap_or_else(|_| "file".to_string()); - - // Strip internal keys so they don't leak into the OpenDAL operator config. - props.remove(PROP_TABLE_IDENT); - - Ok(Arc::new(OpenDalStorage::build_from_props(&scheme, props)?)) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::TableIdent; - use crate::io::StorageCredential; - use crate::io::storage::StorageConfig; - - #[cfg(feature = "storage-memory")] - #[test] - fn test_default_memory_operator() { - let op = default_memory_operator(); - assert_eq!(op.info().scheme().to_string(), "memory"); - } - - #[cfg(feature = "storage-memory")] - #[test] - fn test_build_from_props_memory() { - let storage = OpenDalStorage::build_from_props("memory", HashMap::new()).unwrap(); - match storage { - OpenDalStorage::Memory(_) => {} - _ => panic!("Expected Memory variant"), - } - } - - #[cfg(feature = "storage-s3")] - #[test] - fn test_build_from_props_s3_never_creates_refreshable() { - let mut props = HashMap::new(); - props.insert("bucket".to_string(), "test-bucket".to_string()); - - let storage = OpenDalStorage::build_from_props("s3", props).unwrap(); - - match storage { - OpenDalStorage::S3 { .. } => {} - OpenDalStorage::Refreshable { .. } => { - panic!("build_from_props should not create Refreshable") - } - _ => panic!("Expected S3 variant"), - } - } - - #[derive(Debug)] - struct TestCredentialLoader; - - #[async_trait::async_trait] - impl StorageCredentialsLoader for TestCredentialLoader { - async fn load_credentials( - &self, - _table_ident: &TableIdent, - _location: &str, - ) -> Result { - Ok(StorageCredential { - prefix: "s3://test/".to_string(), - config: HashMap::new(), - }) - } - } - - #[cfg(feature = "storage-s3")] - #[test] - fn test_refreshable_storage_factory_creates_refreshable() { - let loader: Arc = Arc::new(TestCredentialLoader); - let factory = RefreshableStorageFactory::new(loader); - let table_ident = TableIdent::from_strs(["ns", "tbl"]).unwrap(); - let config = StorageConfig::new() - .with_prop(PROP_METADATA_LOCATION, "s3://test-bucket/") - .with_prop( - PROP_TABLE_IDENT, - serde_json::to_string(&table_ident).unwrap(), - ); - assert!( - factory.build(&config).is_ok(), - "RefreshableStorageFactory should build a Refreshable storage successfully" - ); - } - - #[cfg(feature = "storage-s3")] - #[test] - fn test_refreshable_storage_factory_with_initial_credentials_creates_refreshable() { - let loader: Arc = Arc::new(TestCredentialLoader); - let factory = RefreshableStorageFactory::new(loader); - let table_ident = TableIdent::from_strs(["ns", "tbl"]).unwrap(); - // Initial credentials are merged into props before build() is called - let config = StorageConfig::new() - .with_prop(PROP_METADATA_LOCATION, "s3://test-bucket/") - .with_prop( - PROP_TABLE_IDENT, - serde_json::to_string(&table_ident).unwrap(), - ); - assert!( - factory.build(&config).is_ok(), - "RefreshableStorageFactory should build successfully" - ); - } - - #[cfg(feature = "storage-s3")] - #[test] - fn test_routing_factory_routes_to_s3() { - let factory = OpenDalRoutingStorageFactory; - let config = StorageConfig::new() - .with_prop(PROP_METADATA_LOCATION, "s3://test-bucket/path/metadata") - .with_prop("bucket", "test-bucket"); - assert!( - factory.build(&config).is_ok(), - "OpenDalRoutingStorageFactory should route s3:// to S3 storage" - ); - } - - #[cfg(feature = "storage-memory")] - #[test] - fn test_routing_factory_routes_to_memory() { - let factory = OpenDalRoutingStorageFactory; - let config = - StorageConfig::new().with_prop(PROP_METADATA_LOCATION, "memory:/path/metadata"); - assert!( - factory.build(&config).is_ok(), - "OpenDalRoutingStorageFactory should route memory:/ to Memory storage" - ); - } - - #[test] - fn test_routing_factory_errors_on_missing_location() { - let factory = OpenDalRoutingStorageFactory; - let config = StorageConfig::new(); - assert!( - factory.build(&config).is_err(), - "OpenDalRoutingStorageFactory should error when metadata location is missing" - ); - } -} diff --git a/crates/iceberg/src/io/storage/opendal/oss.rs b/crates/iceberg/src/io/storage/opendal/oss.rs deleted file mode 100644 index 9f45c803ef..0000000000 --- a/crates/iceberg/src/io/storage/opendal/oss.rs +++ /dev/null @@ -1,56 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::collections::HashMap; - -use opendal::services::OssConfig; -use opendal::{Configurator, Operator}; -use url::Url; - -use super::super::config::{OSS_ACCESS_KEY_ID, OSS_ACCESS_KEY_SECRET, OSS_ENDPOINT}; -use crate::{Error, ErrorKind, Result}; - -/// Parse iceberg props to oss config. -pub(crate) fn oss_config_parse(mut m: HashMap) -> Result { - let mut cfg: OssConfig = OssConfig::default(); - if let Some(endpoint) = m.remove(OSS_ENDPOINT) { - cfg.endpoint = Some(endpoint); - }; - if let Some(access_key_id) = m.remove(OSS_ACCESS_KEY_ID) { - cfg.access_key_id = Some(access_key_id); - }; - if let Some(access_key_secret) = m.remove(OSS_ACCESS_KEY_SECRET) { - cfg.access_key_secret = Some(access_key_secret); - }; - - Ok(cfg) -} - -/// Build new opendal operator from give path. -pub(crate) fn oss_config_build(cfg: &OssConfig, path: &str) -> Result { - let url = Url::parse(path)?; - let bucket = url.host_str().ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Invalid oss url: {path}, missing bucket"), - ) - })?; - - let builder = cfg.clone().into_builder().bucket(bucket); - - Ok(Operator::new(builder)?) -} diff --git a/crates/iceberg/src/io/storage/opendal/s3.rs b/crates/iceberg/src/io/storage/opendal/s3.rs deleted file mode 100644 index f5cfdaccc7..0000000000 --- a/crates/iceberg/src/io/storage/opendal/s3.rs +++ /dev/null @@ -1,220 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::collections::HashMap; -use std::sync::Arc; - -use async_trait::async_trait; -use opendal::services::S3Config; -use opendal::{Configurator, Operator}; -pub use reqsign::{AwsCredential, AwsCredentialLoad}; -use reqwest::Client; -use url::Url; - -use super::super::config::{ - CLIENT_REGION, S3_ACCESS_KEY_ID, S3_ALLOW_ANONYMOUS, S3_ASSUME_ROLE_ARN, - S3_ASSUME_ROLE_EXTERNAL_ID, S3_ASSUME_ROLE_SESSION_NAME, S3_DISABLE_CONFIG_LOAD, - S3_DISABLE_EC2_METADATA, S3_ENDPOINT, S3_PATH_STYLE_ACCESS, S3_REGION, S3_SECRET_ACCESS_KEY, - S3_SESSION_TOKEN, S3_SSE_KEY, S3_SSE_MD5, S3_SSE_TYPE, -}; -use crate::io::is_truthy; -use crate::{Error, ErrorKind, Result}; - -/// Parse iceberg props to s3 config. -pub(crate) fn s3_config_parse(mut m: HashMap) -> Result { - let mut cfg = S3Config::default(); - if let Some(endpoint) = m.remove(S3_ENDPOINT) { - cfg.endpoint = Some(endpoint); - }; - if let Some(access_key_id) = m.remove(S3_ACCESS_KEY_ID) { - cfg.access_key_id = Some(access_key_id); - }; - if let Some(secret_access_key) = m.remove(S3_SECRET_ACCESS_KEY) { - cfg.secret_access_key = Some(secret_access_key); - }; - if let Some(session_token) = m.remove(S3_SESSION_TOKEN) { - cfg.session_token = Some(session_token); - }; - if let Some(region) = m.remove(S3_REGION) { - cfg.region = Some(region); - }; - if let Some(region) = m.remove(CLIENT_REGION) { - cfg.region = Some(region); - }; - if let Some(path_style_access) = m.remove(S3_PATH_STYLE_ACCESS) { - cfg.enable_virtual_host_style = !is_truthy(path_style_access.to_lowercase().as_str()); - }; - if let Some(arn) = m.remove(S3_ASSUME_ROLE_ARN) { - cfg.role_arn = Some(arn); - } - if let Some(external_id) = m.remove(S3_ASSUME_ROLE_EXTERNAL_ID) { - cfg.external_id = Some(external_id); - }; - if let Some(session_name) = m.remove(S3_ASSUME_ROLE_SESSION_NAME) { - cfg.role_session_name = Some(session_name); - }; - let s3_sse_key = m.remove(S3_SSE_KEY); - if let Some(sse_type) = m.remove(S3_SSE_TYPE) { - match sse_type.to_lowercase().as_str() { - // No Server Side Encryption - "none" => {} - // S3 SSE-S3 encryption (S3 managed keys). https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingServerSideEncryption.html - "s3" => { - cfg.server_side_encryption = Some("AES256".to_string()); - } - // S3 SSE KMS, either using default or custom KMS key. https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingKMSEncryption.html - "kms" => { - cfg.server_side_encryption = Some("aws:kms".to_string()); - cfg.server_side_encryption_aws_kms_key_id = s3_sse_key; - } - // S3 SSE-C, using customer managed keys. https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerSideEncryptionCustomerKeys.html - "custom" => { - cfg.server_side_encryption_customer_algorithm = Some("AES256".to_string()); - cfg.server_side_encryption_customer_key = s3_sse_key; - cfg.server_side_encryption_customer_key_md5 = m.remove(S3_SSE_MD5); - } - _ => { - return Err(Error::new( - ErrorKind::DataInvalid, - format!( - "Invalid {S3_SSE_TYPE}: {sse_type}. Expected one of (custom, kms, s3, none)" - ), - )); - } - } - }; - - if let Some(allow_anonymous) = m.remove(S3_ALLOW_ANONYMOUS) - && is_truthy(allow_anonymous.to_lowercase().as_str()) - { - cfg.skip_signature = true; - } - if let Some(disable_ec2_metadata) = m.remove(S3_DISABLE_EC2_METADATA) - && is_truthy(disable_ec2_metadata.to_lowercase().as_str()) - { - cfg.disable_ec2_metadata = true; - }; - if let Some(disable_config_load) = m.remove(S3_DISABLE_CONFIG_LOAD) - && is_truthy(disable_config_load.to_lowercase().as_str()) - { - cfg.disable_config_load = true; - }; - - Ok(cfg) -} - -/// Build new opendal operator from give path. -pub(crate) fn s3_config_build( - cfg: &S3Config, - customized_credential_load: &Option, - path: &str, -) -> Result { - let url = Url::parse(path)?; - let bucket = url.host_str().ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Invalid s3 url: {path}, missing bucket"), - ) - })?; - - let mut builder = cfg - .clone() - .into_builder() - // Set bucket name. - .bucket(bucket); - - if let Some(customized_credential_load) = customized_credential_load { - let chain = reqsign_core::ProvideCredentialChain::new().push( - CustomAwsCredentialLoaderBridge(customized_credential_load.clone()), - ); - builder = builder.credential_provider_chain(chain); - } - - Ok(Operator::new(builder)?) -} - -/// Custom AWS credential loader. -/// This can be used to load credentials from a custom source, such as the AWS SDK. -/// -/// This should be set as an extension on `FileIOBuilder`. -#[derive(Clone)] -pub struct CustomAwsCredentialLoader(Arc); - -impl std::fmt::Debug for CustomAwsCredentialLoader { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("CustomAwsCredentialLoader") - .finish_non_exhaustive() - } -} - -impl CustomAwsCredentialLoader { - /// Create a new custom AWS credential loader. - pub fn new(loader: Arc) -> Self { - Self(loader) - } - - /// Convert this loader into an opendal compatible loader for customized AWS credentials. - pub fn into_opendal_loader(self) -> Box { - Box::new(self) - } -} - -#[async_trait] -impl AwsCredentialLoad for CustomAwsCredentialLoader { - async fn load_credential(&self, client: Client) -> anyhow::Result> { - self.0.load_credential(client).await - } -} - -/// Bridges a [`CustomAwsCredentialLoader`] (built against the old `reqsign` v0.16 -/// `AwsCredentialLoad` trait, which the fork's vended-credentials feature is written -/// against) into `reqsign_core::ProvideCredential`, which is what opendal's S3 service -/// backend now requires (it moved to the `reqsign-core`/`reqsign-aws-v4` crate family). -/// These are two independent major versions of reqsign with incompatible credential -/// traits; there is no adapter provided upstream, so this constructs a fresh -/// `reqwest::Client` per call (the old trait requires one) and converts the returned -/// credential's fields across the two (structurally identical) `Credential` types. -struct CustomAwsCredentialLoaderBridge(CustomAwsCredentialLoader); - -impl std::fmt::Debug for CustomAwsCredentialLoaderBridge { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("CustomAwsCredentialLoaderBridge") - .finish_non_exhaustive() - } -} - -impl reqsign_core::ProvideCredential for CustomAwsCredentialLoaderBridge { - type Credential = reqsign_aws_v4::Credential; - - async fn provide_credential( - &self, - _ctx: &reqsign_core::Context, - ) -> reqsign_core::Result> { - let loaded = self.0.load_credential(Client::new()).await.map_err(|e| { - reqsign_core::Error::new(reqsign_core::ErrorKind::Unexpected, e.to_string()) - })?; - - Ok(loaded.map(|c| reqsign_aws_v4::Credential { - access_key_id: c.access_key_id, - secret_access_key: c.secret_access_key, - session_token: c.session_token, - expires_in: c - .expires_in - .and_then(|dt| reqsign_core::time::Timestamp::from_second(dt.timestamp()).ok()), - })) - } -} diff --git a/crates/storage/refreshable/public-api.txt b/crates/storage/refreshable/public-api.txt deleted file mode 100644 index fafbcce8df..0000000000 --- a/crates/storage/refreshable/public-api.txt +++ /dev/null @@ -1,34 +0,0 @@ -pub mod iceberg_storage_refreshable -pub struct iceberg_storage_refreshable::RefreshableStorage -impl core::clone::Clone for iceberg_storage_refreshable::RefreshableStorage -pub fn iceberg_storage_refreshable::RefreshableStorage::clone(&self) -> Self -impl core::fmt::Debug for iceberg_storage_refreshable::RefreshableStorage -pub fn iceberg_storage_refreshable::RefreshableStorage::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::storage::Storage for iceberg_storage_refreshable::RefreshableStorage -pub fn iceberg_storage_refreshable::RefreshableStorage::delete<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::delete_prefix<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::delete_stream<'life0, 'async_trait>(&'life0 self, paths: futures_core::stream::BoxStream<'static, alloc::string::String>) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::exists<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::metadata<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::new_input(&self, path: &str) -> iceberg::error::Result -pub fn iceberg_storage_refreshable::RefreshableStorage::new_output(&self, path: &str) -> iceberg::error::Result -pub fn iceberg_storage_refreshable::RefreshableStorage::read<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::reader<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::write<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str, bs: bytes::bytes::Bytes) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -pub fn iceberg_storage_refreshable::RefreshableStorage::writer<'life0, 'life1, 'async_trait>(&'life0 self, path: &'life1 str) -> core::pin::Pin>> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait, 'life1: 'async_trait -impl serde_core::ser::Serialize for iceberg_storage_refreshable::RefreshableStorage -pub fn iceberg_storage_refreshable::RefreshableStorage::serialize(&self, serializer: S) -> core::result::Result<::Ok, ::Error> where S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg_storage_refreshable::RefreshableStorage -pub fn iceberg_storage_refreshable::RefreshableStorage::deserialize(deserializer: D) -> core::result::Result::Error> where D: serde_core::de::Deserializer<'de> -pub struct iceberg_storage_refreshable::RefreshableStorageFactory -impl iceberg_storage_refreshable::RefreshableStorageFactory -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::new(credentials_loader: alloc::sync::Arc) -> Self -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::with_factory(self, factory: alloc::sync::Arc) -> Self -impl core::fmt::Debug for iceberg_storage_refreshable::RefreshableStorageFactory -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl iceberg::io::storage::StorageFactory for iceberg_storage_refreshable::RefreshableStorageFactory -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::build(&self, config: &iceberg::io::storage::config::StorageConfig) -> iceberg::error::Result> -impl serde_core::ser::Serialize for iceberg_storage_refreshable::RefreshableStorageFactory -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer -impl<'de> serde_core::de::Deserialize<'de> for iceberg_storage_refreshable::RefreshableStorageFactory -pub fn iceberg_storage_refreshable::RefreshableStorageFactory::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> From 6a766126a85995438b8b680e7395677b769582e0 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Wed, 2 Sep 2026 16:35:07 +0200 Subject: [PATCH 5/8] fix(catalog/rest): point test at iceberg-storage-refreshable 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 --- crates/catalog/rest/Cargo.toml | 1 + crates/catalog/rest/src/catalog.rs | 3 ++- crates/iceberg/src/io/file_io.rs | 11 ++++++----- crates/iceberg/src/io/storage_credential.rs | 4 ++-- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crates/catalog/rest/Cargo.toml b/crates/catalog/rest/Cargo.toml index 8dc9a86d7f..bbafc7fae9 100644 --- a/crates/catalog/rest/Cargo.toml +++ b/crates/catalog/rest/Cargo.toml @@ -47,6 +47,7 @@ uuid = { workspace = true, features = ["v4"] } [dev-dependencies] bytes = { workspace = true } futures = { workspace = true } +iceberg-storage-refreshable = { workspace = true } iceberg_test_utils = { path = "../../test_utils", features = ["tests"] } mockito = { workspace = true } # `stream` lets tests build a streaming body to exercise HttpRequestBody::Streaming. diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 899b744e98..1d21648420 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -4740,7 +4740,8 @@ mod tests { #[tokio::test] async fn test_load_table_with_refreshable_storage_factory() { - use iceberg::io::{RefreshableStorageFactory, StorageCredential, StorageCredentialsLoader}; + use iceberg::io::{StorageCredential, StorageCredentialsLoader}; + use iceberg_storage_refreshable::RefreshableStorageFactory; #[derive(Debug)] struct DummyCredentialLoader; diff --git a/crates/iceberg/src/io/file_io.rs b/crates/iceberg/src/io/file_io.rs index 3fe7074041..3483a65136 100644 --- a/crates/iceberg/src/io/file_io.rs +++ b/crates/iceberg/src/io/file_io.rs @@ -222,9 +222,10 @@ impl FileIOBuilder { /// Set the runtime table identity context. /// - /// Serializes the `TableIdent` as JSON into a well-known prop key so that - /// `RefreshableStorageFactory::build()` can recover it and pass it to the - /// credential loader on refresh. Other storage factories ignore this prop. + /// Serializes the `TableIdent` as JSON into a well-known prop key so that a + /// credential-refreshing `StorageFactory` implementation (e.g. a fork's own + /// vended-credentials wrapper) can recover it and pass it to its credential loader on + /// refresh. Other storage factories ignore this prop. pub fn with_table_ident(mut self, table_ident: crate::catalog::TableIdent) -> Self { let json = serde_json::to_string(&table_ident).expect("TableIdent serialization is infallible"); @@ -234,8 +235,8 @@ impl FileIOBuilder { /// Set the runtime metadata location context. /// - /// Stores the location under a well-known prop key so that - /// `RefreshableStorageFactory::build()` can pass it to the credential loader on refresh. + /// Stores the location under a well-known prop key so that a credential-refreshing + /// `StorageFactory` implementation can pass it to its credential loader on refresh. /// Other storage factories ignore this prop. pub fn with_location(mut self, location: impl Into) -> Self { self.config = self diff --git a/crates/iceberg/src/io/storage_credential.rs b/crates/iceberg/src/io/storage_credential.rs index d27600d76b..003def6d33 100644 --- a/crates/iceberg/src/io/storage_credential.rs +++ b/crates/iceberg/src/io/storage_credential.rs @@ -32,8 +32,8 @@ pub struct StorageCredential { pub config: HashMap, } -/// Newtype wrapper for the metadata location string, used as an extension -/// so that `RefreshableOpenDalStorage` can pass it to `load_credentials`. +/// Newtype wrapper for the metadata location string, used as an extension so a +/// credential-refreshing `StorageFactory` implementation can pass it to `load_credentials`. #[derive(Debug, Clone)] pub struct MetadataLocation(pub String); From 7f4d5d218481165bb2543ee5a924931b582de4ed Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Thu, 3 Sep 2026 09:45:59 +0200 Subject: [PATCH 6/8] fix(storage/refreshable): retry every reader() read, not just construction 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 -- 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 --- crates/storage/refreshable/src/lib.rs | 134 +++++++++++++++++++++++--- 1 file changed, 123 insertions(+), 11 deletions(-) diff --git a/crates/storage/refreshable/src/lib.rs b/crates/storage/refreshable/src/lib.rs index d059ec596d..7e517506df 100644 --- a/crates/storage/refreshable/src/lib.rs +++ b/crates/storage/refreshable/src/lib.rs @@ -109,6 +109,50 @@ impl State { } } +/// Wraps the [`FileRead`] a backend returns from `reader()` so that *every* +/// subsequent `.read()` call gets the same refresh-and-retry-once protection as a +/// direct [`RefreshableStorage`] method call — not just the call that constructed it. +/// +/// This matters because a reader is typically obtained once and then used for many +/// range reads over the lifetime of a long read (e.g. a Parquet scan reading many row +/// groups) — exactly the scenario a credential expiring mid-flight needs to survive, +/// and exactly what retrying only the initial `reader()` construction would miss. +/// +/// Each `.read()` call resolves the current backend and constructs a fresh inner +/// reader from it, rather than holding one open across calls. This has no real cost: +/// every `Storage` method (including `reader()` itself) already rebuilds its +/// backend's operator from config on each call, so a `Storage::reader(path)` call is +/// already exactly this cheap per invocation — and range reads are naturally +/// idempotent, so re-resolving per call is safe. It also avoids needing a lock that +/// would otherwise serialize concurrent range reads through one shared reader. +struct RefreshableReader { + state: Arc, + path: String, +} + +#[async_trait] +impl FileRead for RefreshableReader { + async fn read(&self, range: std::ops::Range) -> Result { + let (backend, version) = self.state.current(); + let attempt: Result = + async { backend.reader(&self.path).await?.read(range.clone()).await }.await; + + match attempt { + Ok(v) => Ok(v), + Err(_) => { + self.state.refresh(version).await?; + self.state + .current() + .0 + .reader(&self.path) + .await? + .read(range) + .await + } + } + } +} + /// A [`Storage`] that wraps another [`Storage`] (by default, any backend /// [`iceberg-storage-opendal`](iceberg_storage_opendal) can build) and transparently /// refreshes credentials on failure. @@ -129,10 +173,22 @@ impl State { /// predecessor (an opendal-accessor-level wrapper), which had no error-kind filter /// either. /// -/// `delete_stream` is the one exception: the input `Stream` is consumed by the first +/// `delete_stream` is one exception: the input `Stream` is consumed by the first /// attempt and can't be safely replayed, so it is never retried — a failure always /// propagates immediately, matching this crate's predecessor's already-accepted lack of /// retry for several stream-based operations. +/// +/// `reader`/`writer` are handled specially, since both return a long-lived object +/// rather than a single result: +/// - `reader()`'s returned [`FileRead`] is wrapped ([`RefreshableReader`]) so every +/// subsequent `.read()` call gets this same retry-once treatment, not just the call +/// that constructed it — this crate's predecessor gave every read this protection +/// for a reader's whole lifetime (via an opendal-level accessor wrapper), and a +/// long-lived reader used for many range reads (e.g. a Parquet scan) is exactly the +/// scenario this whole mechanism exists to protect. +/// - `writer()`'s returned [`FileWrite`] is NOT wrapped this way — see its own doc +/// comment for why a stateful writer can't safely retry a failed write against a +/// freshly-constructed one. pub struct RefreshableStorage { /// `None` only after deserialization, which cannot reconstruct live credential state /// (the credentials loader and current backend are never serializable) — every method @@ -245,15 +301,11 @@ impl Storage for RefreshableStorage { } async fn reader(&self, path: &str) -> Result> { - let state = self.state()?; - let (backend, version) = state.current(); - match backend.reader(path).await { - Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.reader(path).await - } - } + let state = Arc::clone(self.state()?); + Ok(Box::new(RefreshableReader { + state, + path: path.to_string(), + })) } async fn write(&self, path: &str, bs: Bytes) -> Result<()> { @@ -268,6 +320,15 @@ impl Storage for RefreshableStorage { } } + /// Unlike `reader()`, the returned [`FileWrite`] is NOT wrapped with per-call + /// retry — only this construction call is retried. A writer is frequently + /// stateful (e.g. a multipart upload accumulating state across `.write()` calls), + /// so a write that fails partway through can't be safely retried by constructing + /// a fresh writer and replaying just the failed call: any bytes already + /// successfully written to the old, now-abandoned writer would be silently lost + /// rather than replayed, potentially producing truncated output. Reads don't + /// have this problem — a range read is naturally idempotent, so retrying it + /// against a freshly-constructed reader is always safe. async fn writer(&self, path: &str) -> Result> { let state = self.state()?; let (backend, version) = state.current(); @@ -456,7 +517,10 @@ mod tests { unimplemented!() } async fn reader(&self, _path: &str) -> Result> { - unimplemented!() + Ok(Box::new(FakeReader { + version: self.version, + fail_until_version: self.fail_until_version, + })) } async fn write(&self, _path: &str, _bs: Bytes) -> Result<()> { unimplemented!() @@ -481,6 +545,28 @@ mod tests { } } + /// A [`FileRead`] whose `.read()` succeeds only once `version >= fail_until_version` + /// — fixed at construction time (mirrors the version the *backend* that built it was + /// built with), so a reader obtained before a refresh keeps failing on every `.read()` + /// call even after a later, separately-obtained backend would succeed. This is what + /// makes `reader_retries_individual_read_calls_not_just_construction` a meaningful + /// test of [`RefreshableReader`] specifically, not just of `reader()`'s construction. + struct FakeReader { + version: u64, + fail_until_version: u64, + } + + #[async_trait] + impl FileRead for FakeReader { + async fn read(&self, _range: std::ops::Range) -> Result { + if self.version < self.fail_until_version { + Err(Error::new(ErrorKind::Unexpected, "stale credentials")) + } else { + Ok(Bytes::from_static(b"data")) + } + } + } + #[derive(Debug, Clone, Serialize, Deserialize)] struct FakeStorageFactory { fail_until_version: u64, @@ -609,4 +695,30 @@ mod tests { assert!(input.exists().await.unwrap()); assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); } + + #[tokio::test] + async fn reader_retries_individual_read_calls_not_just_construction() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let storage = build(1, Arc::clone(&loader)); + + // Obtain the reader *before* any refresh has happened -- reader() itself + // always succeeds immediately (construction is deferred), so this must not + // trigger a refresh on its own. + let reader = storage.reader("x").await.unwrap(); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 0); + + // The first .read() call is what actually hits the (still-stale) backend and + // must itself trigger exactly one refresh-and-retry -- proving retry isn't + // limited to whatever call constructed the reader. + let data = reader.read(0..4).await.unwrap(); + assert_eq!(data, Bytes::from_static(b"data")); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + + // A second .read() call on the *same*, already-obtained reader must observe + // the already-refreshed state and need no further refresh. + reader.read(0..4).await.unwrap(); + assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); + } } From d6a08e95636a86b70380f4eaf6dd9666ecc68987 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Thu, 3 Sep 2026 12:05:36 +0200 Subject: [PATCH 7/8] fix(storage/refreshable): preserve original error, add missing test coverage 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 --- crates/storage/refreshable/src/lib.rs | 183 +++++++++++++++++++++----- 1 file changed, 153 insertions(+), 30 deletions(-) diff --git a/crates/storage/refreshable/src/lib.rs b/crates/storage/refreshable/src/lib.rs index 7e517506df..ef880a5e2d 100644 --- a/crates/storage/refreshable/src/lib.rs +++ b/crates/storage/refreshable/src/lib.rs @@ -35,6 +35,7 @@ use std::collections::HashMap; use std::fmt; +use std::future::Future; use std::sync::{Arc, RwLock}; use async_trait::async_trait; @@ -139,15 +140,12 @@ impl FileRead for RefreshableReader { match attempt { Ok(v) => Ok(v), - Err(_) => { - self.state.refresh(version).await?; - self.state - .current() - .0 - .reader(&self.path) - .await? - .read(range) - .await + Err(e) => { + let path = self.path.clone(); + refresh_and_retry(&self.state, version, e, move |b| async move { + b.reader(&path).await?.read(range).await + }) + .await } } } @@ -261,6 +259,29 @@ impl RefreshableStorage { } } +/// Refreshes credentials (double-checked against `seen_version`) and retries `op` +/// against the freshly-refreshed backend. If `op` also fails, `original_err` is +/// attached as context on the final error — so a caller debugging the failure can see +/// what the *first* attempt failed with too, not just whatever the retry hit, which +/// may be an unrelated, more confusing symptom (e.g. credentials refresh successfully +/// but the retry then 404s for a genuinely separate reason). +async fn refresh_and_retry( + state: &State, + seen_version: u64, + original_err: Error, + op: F, +) -> Result +where + F: FnOnce(Arc) -> Fut, + Fut: Future>, +{ + state.refresh(seen_version).await?; + let (backend, _) = state.current(); + op(backend) + .await + .map_err(|e| e.with_context("original_error", original_err.to_string())) +} + #[async_trait] #[typetag::serde(name = "RefreshableStorage")] impl Storage for RefreshableStorage { @@ -269,9 +290,8 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.exists(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.exists(path).await + Err(e) => { + refresh_and_retry(state, version, e, |b| async move { b.exists(path).await }).await } } } @@ -281,9 +301,9 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.metadata(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.metadata(path).await + Err(e) => { + refresh_and_retry(state, version, e, |b| async move { b.metadata(path).await }) + .await } } } @@ -293,9 +313,8 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.read(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.read(path).await + Err(e) => { + refresh_and_retry(state, version, e, |b| async move { b.read(path).await }).await } } } @@ -313,9 +332,14 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.write(path, bs.clone()).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.write(path, bs).await + Err(e) => { + refresh_and_retry( + state, + version, + e, + |b| async move { b.write(path, bs).await }, + ) + .await } } } @@ -334,9 +358,8 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.writer(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.writer(path).await + Err(e) => { + refresh_and_retry(state, version, e, |b| async move { b.writer(path).await }).await } } } @@ -346,9 +369,8 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.delete(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.delete(path).await + Err(e) => { + refresh_and_retry(state, version, e, |b| async move { b.delete(path).await }).await } } } @@ -358,9 +380,14 @@ impl Storage for RefreshableStorage { let (backend, version) = state.current(); match backend.delete_prefix(path).await { Ok(v) => Ok(v), - Err(_) => { - state.refresh(version).await?; - state.current().0.delete_prefix(path).await + Err(e) => { + refresh_and_retry( + state, + version, + e, + |b| async move { b.delete_prefix(path).await }, + ) + .await } } } @@ -721,4 +748,100 @@ mod tests { reader.read(0..4).await.unwrap(); assert_eq!(loader.call_count.load(Ordering::SeqCst), 1); } + + #[tokio::test] + async fn retry_failure_preserves_original_error_as_context() { + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + // Never succeeds, no matter how many times it's refreshed -- so the retry + // fails too, and its error must still mention the *original* failure. + let storage = build(u64::MAX, Arc::clone(&loader)); + + let err = storage.exists("x").await.unwrap_err(); + let rendered = format!("{err:?}"); + assert!( + rendered.contains("original_error"), + "error should carry the original attempt's failure as context: {rendered}" + ); + assert!( + rendered.contains("stale credentials"), + "context should contain the original error's message: {rendered}" + ); + } + + #[tokio::test] + async fn write_and_read_roundtrip_through_real_backend() { + // Unlike the other tests here, this goes through a real + // iceberg-storage-opendal backend (in-memory) rather than FakeStorage, to + // prove write()/read() actually work end-to-end through this wrapper, not + // just that the retry bookkeeping around them is correct. + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let factory = RefreshableStorageFactory::new(loader).with_factory(Arc::new( + iceberg_storage_opendal::OpenDalStorageFactory::Memory, + )); + let storage = factory.build(&StorageConfig::new()).unwrap(); + + storage + .write("memory:/roundtrip-file", Bytes::from_static(b"hello")) + .await + .unwrap(); + let data = storage.read("memory:/roundtrip-file").await.unwrap(); + assert_eq!(data, Bytes::from_static(b"hello")); + } + + #[tokio::test] + async fn factory_build_parses_table_ident_and_location_from_props() { + #[derive(Debug)] + struct CapturingLoader { + captured: std::sync::Mutex>, + } + + #[async_trait] + impl StorageCredentialsLoader for CapturingLoader { + async fn load_credentials( + &self, + table_ident: &TableIdent, + location: &str, + ) -> Result { + *self.captured.lock().unwrap() = Some((table_ident.clone(), location.to_string())); + Ok(StorageCredential { + prefix: String::new(), + config: HashMap::from([("version".to_string(), "1".to_string())]), + }) + } + } + + let loader = Arc::new(CapturingLoader { + captured: std::sync::Mutex::new(None), + }); + let table_ident = TableIdent::from_strs(["ns", "tbl"]).unwrap(); + // This is exactly what FileIOBuilder::with_table_ident/with_location populate. + let config = StorageConfig::new() + .with_prop( + PROP_METADATA_LOCATION, + "s3://test-bucket/table/metadata.json", + ) + .with_prop( + PROP_TABLE_IDENT, + serde_json::to_string(&table_ident).unwrap(), + ); + + let factory = RefreshableStorageFactory::new(Arc::clone(&loader) as _).with_factory( + Arc::new(FakeStorageFactory { + fail_until_version: 1, + }), + ); + let storage = factory.build(&config).unwrap(); + + // fail_until_version: 1 guarantees the first call fails and triggers exactly + // one refresh, so the loader is actually invoked by the time we assert below. + storage.exists("x").await.unwrap(); + + let (captured_ident, captured_location) = loader.captured.lock().unwrap().clone().unwrap(); + assert_eq!(captured_ident, table_ident); + assert_eq!(captured_location, "s3://test-bucket/table/metadata.json"); + } } From 7f9abe1d382d885a2dbf5cf2dfc1d6992bb9d7f6 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Thu, 3 Sep 2026 12:13:39 +0200 Subject: [PATCH 8/8] fix(storage/refreshable): reuse reader across successful reads 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 --- crates/storage/refreshable/src/lib.rs | 110 +++++++++++++++++++++----- 1 file changed, 89 insertions(+), 21 deletions(-) diff --git a/crates/storage/refreshable/src/lib.rs b/crates/storage/refreshable/src/lib.rs index ef880a5e2d..8cc567089d 100644 --- a/crates/storage/refreshable/src/lib.rs +++ b/crates/storage/refreshable/src/lib.rs @@ -119,33 +119,39 @@ impl State { /// groups) — exactly the scenario a credential expiring mid-flight needs to survive, /// and exactly what retrying only the initial `reader()` construction would miss. /// -/// Each `.read()` call resolves the current backend and constructs a fresh inner -/// reader from it, rather than holding one open across calls. This has no real cost: -/// every `Storage` method (including `reader()` itself) already rebuilds its -/// backend's operator from config on each call, so a `Storage::reader(path)` call is -/// already exactly this cheap per invocation — and range reads are naturally -/// idempotent, so re-resolving per call is safe. It also avoids needing a lock that -/// would otherwise serialize concurrent range reads through one shared reader. +/// The current inner reader is held across calls behind a `tokio::sync::RwLock` +/// rather than rebuilt on every `.read()` — every `Storage` method call rebuilds its +/// backend's whole `opendal::Operator` from config, so re-resolving *that* on every +/// single range read (as an earlier version of this wrapper did) would multiply that +/// cost by the number of reads in a long scan instead of paying it once. Concurrent +/// reads (e.g. parallel row-group fetches against the same file) share a read lock +/// and aren't serialized on the happy path; only a failure-triggered rebuild briefly +/// takes the write lock to swap in a freshly-built reader. This mirrors this crate's +/// predecessor's own `RetryableReader` design exactly, including its reasoning. struct RefreshableReader { state: Arc, path: String, + inner: tokio::sync::RwLock<(Box, u64)>, } #[async_trait] impl FileRead for RefreshableReader { async fn read(&self, range: std::ops::Range) -> Result { - let (backend, version) = self.state.current(); - let attempt: Result = - async { backend.reader(&self.path).await?.read(range.clone()).await }.await; + let attempt = { self.inner.read().await.0.read(range.clone()).await }; match attempt { Ok(v) => Ok(v), - Err(e) => { - let path = self.path.clone(); - refresh_and_retry(&self.state, version, e, move |b| async move { - b.reader(&path).await?.read(range).await - }) - .await + Err(original_err) => { + let seen_version = self.inner.read().await.1; + self.state.refresh(seen_version).await?; + let (backend, new_version) = self.state.current(); + let new_inner = backend + .reader(&self.path) + .await + .map_err(|e| e.with_context("original_error", original_err.to_string()))?; + let result = new_inner.read(range).await; + *self.inner.write().await = (new_inner, new_version); + result.map_err(|e| e.with_context("original_error", original_err.to_string())) } } } @@ -321,9 +327,23 @@ impl Storage for RefreshableStorage { async fn reader(&self, path: &str) -> Result> { let state = Arc::clone(self.state()?); + let (backend, version) = state.current(); + let (inner, version) = match backend.reader(path).await { + Ok(r) => (r, version), + Err(e) => { + state.refresh(version).await?; + let (backend, version) = state.current(); + let inner = backend + .reader(path) + .await + .map_err(|e2| e2.with_context("original_error", e.to_string()))?; + (inner, version) + } + }; Ok(Box::new(RefreshableReader { state, path: path.to_string(), + inner: tokio::sync::RwLock::new((inner, version)), })) } @@ -525,6 +545,8 @@ mod tests { version: u64, #[serde(skip)] fail_until_version: u64, + #[serde(skip)] + reader_calls: Arc, } #[async_trait] @@ -544,6 +566,7 @@ mod tests { unimplemented!() } async fn reader(&self, _path: &str) -> Result> { + self.reader_calls.fetch_add(1, Ordering::SeqCst); Ok(Box::new(FakeReader { version: self.version, fail_until_version: self.fail_until_version, @@ -597,6 +620,11 @@ mod tests { #[derive(Debug, Clone, Serialize, Deserialize)] struct FakeStorageFactory { fail_until_version: u64, + /// Shared across every `FakeStorage` this factory builds, so a test can + /// count how many times `FakeStorage::reader()` was called -- i.e. how many + /// times the (simulated) backend operator was rebuilt. + #[serde(skip)] + reader_calls: Arc, } #[typetag::serde] @@ -609,6 +637,7 @@ mod tests { Ok(Arc::new(FakeStorage { version, fail_until_version: self.fail_until_version, + reader_calls: Arc::clone(&self.reader_calls), })) } } @@ -641,8 +670,11 @@ mod tests { } fn build(fail_until_version: u64, loader: Arc) -> Arc { - let factory = RefreshableStorageFactory::new(loader) - .with_factory(Arc::new(FakeStorageFactory { fail_until_version })); + let factory = + RefreshableStorageFactory::new(loader).with_factory(Arc::new(FakeStorageFactory { + fail_until_version, + reader_calls: Arc::new(AtomicUsize::new(0)), + })); factory.build(&StorageConfig::new()).unwrap() } @@ -730,9 +762,9 @@ mod tests { }); let storage = build(1, Arc::clone(&loader)); - // Obtain the reader *before* any refresh has happened -- reader() itself - // always succeeds immediately (construction is deferred), so this must not - // trigger a refresh on its own. + // FakeStorage::reader() never itself fails (only .read() calls simulate + // failure), so constructing the reader must not trigger a refresh on its own + // -- even though the backend it's bound to is already stale at this point. let reader = storage.reader("x").await.unwrap(); assert_eq!(loader.call_count.load(Ordering::SeqCst), 0); @@ -832,6 +864,7 @@ mod tests { let factory = RefreshableStorageFactory::new(Arc::clone(&loader) as _).with_factory( Arc::new(FakeStorageFactory { fail_until_version: 1, + reader_calls: Arc::new(AtomicUsize::new(0)), }), ); let storage = factory.build(&config).unwrap(); @@ -844,4 +877,39 @@ mod tests { assert_eq!(captured_ident, table_ident); assert_eq!(captured_location, "s3://test-bucket/table/metadata.json"); } + + #[tokio::test] + async fn reader_reuses_inner_reader_across_successful_reads() { + // Regression test: an earlier version of RefreshableReader constructed a + // fresh inner reader (and therefore rebuilt the whole backend operator) on + // *every* .read() call, which would multiply that cost by the number of + // reads in a long scan instead of paying it once, unlike this crate's + // predecessor. Successful reads must reuse the same inner reader. + let loader = Arc::new(TrackingLoader { + call_count: AtomicUsize::new(0), + }); + let reader_calls = Arc::new(AtomicUsize::new(0)); + let factory = + RefreshableStorageFactory::new(loader).with_factory(Arc::new(FakeStorageFactory { + fail_until_version: 0, + reader_calls: Arc::clone(&reader_calls), + })); + let storage = factory.build(&StorageConfig::new()).unwrap(); + + let reader = storage.reader("x").await.unwrap(); + assert_eq!( + reader_calls.load(Ordering::SeqCst), + 1, + "reader() itself builds the inner reader exactly once" + ); + + for _ in 0..5 { + reader.read(0..4).await.unwrap(); + } + assert_eq!( + reader_calls.load(Ordering::SeqCst), + 1, + "5 successful reads must reuse the same inner reader, not rebuild it each time" + ); + } }