Repository navigation
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#67470 Problem Summary: BE creates every native object storage client from one flat `S3ClientConf`. Azure reuses `ak/sk/bucket` as account/key/container, while region, the AWS role and the AWS credential provider type sit in the same struct and only mean something for S3. `S3ClientFactory` builds AWS and Azure clients, owns one shared client cache, applies the rate-limit wrapper and initializes the AWS SDK. `ObjClientHolder` keeps a mutable conf and client side by side: `reset()` writes them under the lock, but `s3_client_conf()` and `full_s3_path()` read the conf without it, and two concurrent resets that both merged onto the same old conf silently overwrite each other. This is the first step of the BE object storage refactor. It changes how clients are configured and managed without changing any caller-facing input: - Add `io::ObjStorageClientConfig` with shared connection options, a client usage (internal storage / external access) and exactly one backend alternative: `S3ClientConfig` or `AzureClientConfig`. Azure authentication is a variant of methods (SharedKey only today), so a new method is a new alternative instead of another loosely related field. - Add `ObjStorageConfigResolver::from_client_conf()`, the single mapping from the legacy conf. All existing inputs (FE properties, `TS3StorageParam`, `ObjectStoreInfoPB`) still produce an `S3ClientConf` and reach the providers through it. - Split construction into `S3ClientProvider` and `AzureClientProvider`, each with its own cache keyed by the full typed config, dispatched by `ObjStorageClientFactory`. SDK clients are still built outside the cache lock; a losing concurrent candidate is now released after unlocking. The BE rate-limit policy moves next to the factory and is applied by every provider in the same way. - `S3ClientFactory` keeps its public API as a compatibility entry: it validates the legacy conf, keeps the test client hook, and still owns the AWS SDK lifetime, the CA certificate path and the AWS credential helpers. - `ObjClientHolder` now owns an immutable `ObjClientState` (conf, client, generation) and replaces it as a whole. Readers take one snapshot, so the conf and client always come from the same generation. A reset is published only if no other publish happened since it read the state; otherwise it returns `ABORTED` and the holder keeps the newer state. Both callers (`update_s3_resource` and storage vault refresh) already handle a failed reset by logging a warning and keeping the existing file system. Client identity now follows the selected backend: S3 clients are no longer split by bucket (an AWS SDK client is not bound to a bucket), and Azure clients are no longer split by region, token or AWS role fields that the Azure client never read. The provider cache singleton is created after the AWS SDK is initialized, so cached AWS clients are released before `Aws::ShutdownAPI`. Renaming `S3URI`, `S3FileSystem`, `S3FileReader` and `S3FileWriter`, and migrating callers from `S3ClientConf` to the typed config, are left to follow-up changes. ### Release note None ### Check List (For Author) - Test: Unit Test (added, not run) - Added `be/test/io/obj_storage/obj_storage_client_factory_test.cpp` for the resolver mapping, backend-specific identity, credential masking, provider caching, holder snapshots and the concurrent reset conflict. - Updated tests that assigned the holder's private client to use `set_client_for_test`. - BE was not compiled and unit tests were not run in this change; clang-format 16 and `build-support/check-build-hygiene.sh` passed. - Behavior changed: Yes. S3 clients are shared across buckets with the same endpoint and credentials; Azure clients ignore S3-only fields for reuse; a reset that races with another reset on the same holder returns `ABORTED` instead of overwriting it. - Does this need documentation: No
xylaaaaa
requested review from
Gabriel39,
gavinchou,
liaoxin01,
luwei16,
morningman and
yiguolei
as code owners
October 14, 2026 16:07
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: BE UT compilation failed with clang's -Wmissing-designated-field-initializers (-Werror) because ObjStorageConfigResolver::from_client_conf() built ObjStorageClientConfig with a designated initializer that omitted `backend` and assigned it afterwards. Build the backend alternative first and initialize all fields in one designated initializer. ### Release note None ### Check List (For Author) - Test: No need to test (compile fix only; BE was not built locally, clang-format 16 and check-build-hygiene.sh passed) - Behavior changed: No - Does this need documentation: No
Contributor
TPC-H: Total hot run time: 29368 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-DS: Total hot run time: 153478 ms |
Contributor
ClickBench: Total hot run time: 24.56 s |
Contributor
TPC-H: Total hot run time: 29722 ms |
Contributor
TPC-DS: Total hot run time: 152943 ms |
Contributor
ClickBench: Total hot run time: 24.49 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: Align the names introduced by this PR with the agreed design for native Azure authentication, which uses the ObjClient* family next to the existing ObjClientHolder: - ObjStorageClientFactory -> ObjClientFactory (obj_client_factory.{h,cpp}) - ObjStorageClientConfig / ObjStorageClientConfigHash -> ObjClientConfig / ObjClientConfigHash - ObjStorageClientCache -> ObjClientCache (obj_client_cache.h) - obj_storage_client_factory_test.cpp -> obj_client_factory_test.cpp ObjStorageClient (the shared object operation interface in common) and the ObjStorage* IO types are unchanged. This is a pure rename with no behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (pure rename; BE was not built locally, clang-format 16 and check-build-hygiene.sh passed) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29594 ms |
Contributor
TPC-DS: Total hot run time: 152362 ms |
Contributor
ClickBench: Total hot run time: 24.48 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: ObjClientFactory::get_or_create() selected the client provider with std::get_if<S3ClientConfig> and fell back to std::get<AzureClientConfig>. Adding a third alternative to ObjClientConfig::backend would still compile, and a config of the new type would throw bad_variant_access at runtime. Dispatch through std::visit to one private overload per backend instead, so a missing overload is a compile error, as the hash and to_string helpers of ObjClientConfig already do. ### Release note None ### Check List (For Author) - Test: No need to test (dispatch is equivalent for both existing alternatives and covered by obj_client_factory_test; BE was not built locally, clang-format 16 and check-build-hygiene.sh passed) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
…ntials ### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: FE passes Azure access to BE through the S3-style properties map, reusing AWS_ACCESS_KEY / AWS_SECRET_KEY for the account and key. That map cannot express a SAS token or Entra ID client credentials, so the native BE Azure client only supports SharedKey, and OAuth2 storage access only works for OneLake through the Hadoop path. This change adds a typed FE-to-BE protocol and the BE side of it: - gensrc: TAzureClientConfig with a TAzureCredential union (shared_key, sas, client_secret), carried by the new optional field TFileScanRangeParams.azure_client_config (id 42, unused on master and on branch-4.2/4.1/4.0/3.1). - AzureAuthFactory builds SAS clients (token appended to the container URL) and client secret clients (Azure::Identity::ClientSecretCredential sharing the blob transport). - AzureAuthConfig gains AzureSasConfig and AzureClientSecretConfig; hash/to_string cover them and keep secrets masked. - AzureConfigCodec decodes TAzureClientConfig (exactly one credential; container from the file location unless configured, and both must match); AzureConfigValidator checks the fields of the selected method and the account against standard Azure hosts; AzureCredentialValidator parses the SAS "se" expiry and rejects an expired SAS. AzureClientProvider runs it on every client request, i.e. each reader open, never per range read. - ObjStorageConfigResolver::from_azure_client_config() is the entry for the typed input. FileFactory uses it for FILE_S3 reads when the scan params carry azure_client_config, and ignores the S3-style keys then. - ObjClientHolder now keeps the typed ObjClientConfig in its state, plus the legacy conf when built from one; a holder built from a typed config initializes through ObjClientFactory directly. - The scan readers that build FileSystemProperties from TFileScanRangeParams copy azure_client_config. ### Release note None ### Check List (For Author) - Test: Unit Test (added, not run; BE was not built locally, clang-format 16 and check-build-hygiene.sh passed) - azure_config_test.cpp: codec, validator, SAS expiry parsing, expired SAS, resolver entry, masking and identity - obj_client_factory_test.cpp: typed holder initialization and an expired SAS rejected before any client is built - Behavior changed: No for existing requests; only requests that carry the new field take the new path. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: FE hands Azure access to BE as an S3-style map (AWS_ACCESS_KEY = account, AWS_SECRET_KEY = key). That map has no way to carry a SAS token or Entra ID client credentials, so FE offered no SAS option at all, and OAuth2 storage access to a normal Azure Blob location reached BE as a Hadoop configuration it cannot use on the FILE_S3 path. This change fills the new TAzureClientConfig from FE: - New user property azure.sas_token (sensitive) with azure.auth_type = SAS; validation requires the token and an endpoint or account name. FE listing (AzureObjStorage) uses the SAS too, and the Hadoop map configures hadoop-azure's fixed SAS token for ABFS access. - AzureFileSystemProperties.toMap() keeps its S3-style keys for old BEs (SAS adds no legacy credential, so an old BE is never handed a wrong one) and also writes neutral AZURE_* keys (AzureBackendPropertyKeys in fe-filesystem-api). OAuth2 resolves the tenant as the FE SDK client already does and derives the authority host from oauth2_server_uri; the FE SDK client now uses that authority host as well. - AzureThriftAdapter (fe-core) is the only place that knows the Thrift shape: it turns the neutral keys into TAzureClientConfig. FileQueryScanNode sets it on TFileScanRangeParams for FILE_S3 ranges, which covers TVF scans and plugin-driven catalogs, since both build their location properties from the provider map. Rolling upgrade: SharedKey keeps working on old BEs through the S3-style keys. SAS and OAuth2 on FILE_S3 need every BE upgraded first. Outbound requests: with OAuth2, BE now requests tokens from the authority host derived from azure.oauth2_server_uri, in addition to the storage endpoint. Both come from catalog/TVF properties, so reaching them requires the privilege to create or alter that catalog, or to run the TVF. ### Release note Azure storage accessed through TVF or external catalogs supports SAS tokens (azure.auth_type = SAS, azure.sas_token) and native BE OAuth2 client credentials, in addition to SharedKey. Upgrade all BEs before using SAS or OAuth2. ### Check List (For Author) - Test: Unit Test (added, not run; FE was not built locally) - AzureFileSystemPropertiesTest: native keys for SharedKey, SAS and OAuth2, SAS validation, SAS hadoop config, sensitive SAS key - AzureThriftAdapterTest: each credential, no native keys, unknown auth type - Behavior changed: Yes. The Azure backend map carries AZURE_* keys and FILE_S3 scans carry TAzureClientConfig; new SAS auth type. - Does this need documentation: Yes (azure.auth_type = SAS and azure.sas_token), not included in this change
Contributor
Author
|
run buildall |
xylaaaaa
added a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 3, 2026
…onfiguration Issue Number: None Related PR: apache#69082 Problem Summary: Every BE object storage entry (FE properties, cold-storage resources, storage vaults) produced one flat `S3Conf` / `S3ClientConf` that carried all providers in a single shape: Azure reused ak/sk/bucket as account/key/container, while region, role_arn and the credential provider type only meant something for S3. `S3ClientFactory::create` then branched on the provider to build an S3 or an Azure client, and `ObjClientHolder` merged credential refreshes field by field into that flat struct. A backend-specific field could leak into the other backend's identity or validation (for example, an Azure configuration still needed a region), and adding an Azure authentication method would have meant more flat fields that S3 must ignore. This is the first step of splitting apache#69082 (BE object storage refactor and native Azure SAS / OAuth2 access). It only restructures the BE; no FE change, no Thrift change, and Azure still supports only SharedKey. - Typed configuration. `io::ObjClientConfig` holds the shared connection options, the client usage (internal storage / external access) and exactly one backend, `std::variant<S3ClientConfig, AzureClientConfig>`. The Azure authentication is a variant as well, with SharedKey as its only alternative for now. `io::ObjStorageConfig` (bucket, prefix, client) replaces `S3Conf`; `S3Conf` and `S3ClientConf` are removed. - One conversion point. `ObjStorageConfigResolver::from_properties` (formerly `S3ClientFactory::convert_properties_to_s3_conf`), `from_storage_param` and `from_object_store_info` (formerly `S3Conf::get_s3_conf`) turn every source into the typed configuration. The flat field mapping and its validation are ported unchanged. `merge_update` merges a refreshed resource or vault configuration with the same field rules as the flat merge. - Client construction by provider. `ObjClientFactory` validates every request with the rules of its backend (the checks `S3ClientFactory::create` applied) and dispatches through `std::visit`, so a backend without a branch fails to compile. `S3ClientProvider` and `AzureClientProvider` build and cache their clients outside the cache lock, and one shared function applies the BE rate limiter. `ObjClientFactory` also owns the test hook `set_client_creator_for_test`. `S3ClientFactory` keeps only the AWS SDK lifetime and the AWS helpers (client configuration, credentials provider, CA path), which Kinesis also uses. - Holder. `ObjClientHolder` moves to `io/obj_storage/obj_client_holder.{h,cpp}` and owns an immutable `ObjClientState` (config, client, generation) that it replaces as a whole, so a reader never pairs a new config with an old client. - Locations. `ObjStorageUri::parse_s3` replaces `S3URI` with the same rules. `FileFactory` resolves FILE_S3 locations through one function for the reader, `create_fs` and `create_file_writer`; the other callers (export, snapshot, connectivity test, cold storage, storage vaults, AI embed, Kinesis, benchmarks, spill) use the resolver. - GCP IAM credentials (apache#68540) move with the S3 fields: `S3AuthConfig::credential` carries the provider-native `CredentialConfig`, the resolver parses and converts it for all three sources and validates it as before, `ObjClientFactory` validates it on every request, and `S3ClientProvider` builds the client through `make_s3_client` with the GCS V4 signed URL generator. A reset takes the vendor and credential from the update, as the flat merge does. - The AWS SDK RFC 3986 encoding flag is set once at SDK initialization instead of in every reader and writer constructor, and the reader no longer uses AWS types for HTTP 429. Behavior changes: 1. The S3 client cache no longer keys on the bucket: different buckets with the same endpoint, credentials and connection options share one client. Building an S3 client never used the bucket. 2. A holder reset merged onto a state that another reset already replaced returns `Aborted` instead of overwriting the newer update; the caller retries with its next refresh. 3. A holder can no longer be reset to another backend (InvalidArgument), which is a different storage target. 4. A flat Azure configuration no longer needs a region (FE sent a placeholder). None - Test: Unit Test (new obj_client_factory_test and obj_storage_uri_test, the latter porting the S3URI cases; existing users, including the GCP IAM cases of s3_client_factory_test, migrated, with test configs built through testutil/obj_storage_test_util.h). Not run locally; run in CI. - Behavior changed: Yes, as listed above - Does this need documentation: No
xylaaaaa
added a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 3, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The holder tests in obj_client_factory_test call secret_key_of, but the helper was dropped together with an adjacent test when this PR was split from apache#69082, so the BE unit tests did not compile. The helper is restored. ### Release note None ### Check List (For Author) - Test: Unit Test (compiled and run in CI) - Behavior changed: No - Does this need documentation: No
…torage-client master removed the legacy file scanner (apache#69723): be/src/exec/scan/file_scanner.{h,cpp} and the V1 readers be/src/format/{csv/csv_reader,json/new_json_reader,orc/vorc_reader, parquet/vparquet_reader,text/text_reader}.cpp. They are deleted here as in master. - The V1 rejection of Azure SAS / OAuth2 (FileScanner::_check_storage_access and its test in file_scanner_v2_test.cpp) goes with the V1 scanner: every scan now runs on FileScannerV2, which serves typed Azure access. - The V1 readers' copy of TFileScanRangeParams.azure_client_config has its counterpart on the V2 paths: TableReader, fetch_file_schema (now used for every schema RPC format), the Iceberg reader's delete files, the deletion vector reader and parquet_meta all set it; the Iceberg delete file helper now reads through TableReader with the delete sink's scan params. obj_storage_file_reader.cpp takes master's read IO trace (`s3_get` records with io_ctx) and atomic statistics under the ObjStorageFileReader names. Tests from master moved to the typed client API: s3_file_reader_test.cpp (ObjClientConfig, set_client_for_test, ObjStorageFileReader, AwsSdkManager), json_reader_test.cpp and csv_reader_test.cpp (ObjClientFactory::set_client_creator_for_test with ObjClientConfig).
Contributor
Author
|
run buildall |
…tial check Issue Number: None Related PR: apache#69605 Problem Summary: The previous commit moved the Azure authentication model to common and also made `S3Accessor::init` reject an Azure SharedKey configuration with only one of the account name and the key. In non-vault mode a failed accessor init fails `InstanceRecycler::init`, so one such storage entry would stop every recycle task of the instance, including the ones that do not touch object storage, where before only that storage failed its requests. The meta service already requires both values for Azure when it stores them, so the check guarded nothing that can be stored. Recycler now only shares the authentication type and `AzureAuthFactory` with BE and builds the client exactly as before. `validate_azure_auth` stays the rule BE checks its inputs with; its comment says that Recycler relies on the meta service validation instead. The BE test of the common rules is renamed to `AzureAuthRulesLiveInCommon`. None - Test: No new test (Recycler behaves as before the previous commit; BE UT and cloud UT in CI) - Behavior changed: No (restores the Recycler behavior) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28903 ms |
Contributor
TPC-DS: Total hot run time: 151736 ms |
Contributor
ClickBench: Total hot run time: 24.45 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: Integrate apache/doris master through 7acfd9b. Preserve the upstream header dependencies on the renamed object storage reader. Its implementation and test include the client holder directly. Apply the upstream reverse reach limit of 50 to the renamed filesystem header and retain all other upstream thresholds. ### Release note None ### Check List (For Author) - Test: Built BE (ASAN, Azure enabled) and FE. Ran the focused BE ASAN unit tests (87 passed, 1 JSON timing assertion failures, 0 skipped). Header hygiene, formatting and clang-tidy checks passed. Ran the build-support gate self-tests. Broader results and previously reported cache-fixture crashes were recorded separately. - Behavior changed: No (merge conflict resolution). - Does this need documentation: No.
morningman
pushed a commit
that referenced
this pull request
Nov 4, 2026
…on (#69392) ### What problem does this PR solve? Issue Number: None Related PR: #69082 Problem Summary: **In short.** DELETE, UPDATE and MERGE on an Iceberg table recorded the delete files they write (v2 position delete files and v3 deletion vector files) under the internal `s3://` address BE writes them at, instead of the table's own location (`oss://`, `s3a://`, `abfss://`, ...). Doris could not always read such a table back, and other engines and maintenance procedures resolved those files with the wrong file system. This PR commits delete files under the table's location. It also makes every Iceberg file Doris writes take both of its paths from one place in BE and pass one check in FE, so the next writer cannot repeat the mistake without failing. #### Background Every Iceberg sink FE sends carries two forms of the table's data location: - a normalized one BE can address the storage with (`output_path`): FE rewrites `oss://`, `cos://`, `cosn://`, `obs://`, `bos://`, `gs://`, `s3a://`, `s3n://` and Azure `abfs[s]://` / `wasb[s]://` locations (except OneLake) to `s3://`; - the raw one Iceberg metadata must record: `original_output_path` on `TIcebergTableSink`, `table_location` on `TIcebergDeleteSink`, both on `TIcebergMergeSink`. BE writes each file under the first form and reports it in `TIcebergCommitData.file_path`, which FE writes into the manifests as it is. So the reported path must be in the second form. #### The problem, and what it cost Reproduce on an Iceberg table located at `oss://bucket/...`: ```sql CREATE TABLE t (id INT, v STRING) PROPERTIES ('format-version' = '2'); INSERT INTO t VALUES (1, 'a'), (2, 'b'), (3, 'c'); DELETE FROM t WHERE id = 1; SELECT file_path FROM t$delete_files; -- s3://bucket/.../data/delete_pos_xxx.parquet SELECT file_path FROM t$data_files; -- oss://bucket/.../data/... ``` - Whether Doris can still read the table depends on the catalog and storage binding. In a real Azure ADLS Hadoop catalog test with explicit SharedKey, Doris SELECT failed with `No storage properties found for schema: s3`, and Spark failed with `UnsupportedFileSystemException: No FileSystem for scheme "s3"`. - Other engines and maintenance operations such as `expire_snapshots` / `remove_orphan_files` may resolve the wrong file system from the recorded scheme, depending on the engine and FileIO. Doris's own `remove_orphan_files` refuses to run on such a table (`Cannot determine whether listed and reachable file locations are equivalent`). - Cause: data files were written under `write_path` and committed under `original_write_path` (`VIcebergPartitionWriter`), but `VIcebergDeleteSink` had no such split; it reported the `output_path`-based path it wrote. - Why it went unnoticed: the two writers mapped the paths in different ways, `VIcebergDeleteFileWriter` reported its write path by default, and FE accepted any path. CI did run on such a location: the Iceberg REST fixture's warehouse is `s3a://`, which FE normalizes to `s3://`. But the DML suites only checked that delete file paths contain `/data/delete_pos_`, and a stray `test_iceberg_update_delete_basic.out.org` in the repository records the wrong `s3://warehouse/wh/...` paths. #### How this PR fixes it 1. `[fix](be) Commit Iceberg delete files under the table's own location`, `[test](be) Verify Iceberg delete file commit paths` and `[test](be) Initialize the Arrow memory pool in the Iceberg position delete write test`: `VIcebergDeleteSink` writes under `output_path` and commits position delete files and deletion vectors under `table_location`. An FE unit test pins that both sinks carrying `table_location` name the same directory as `output_path`, and `DataSinks.thrift` documents the contract. The position delete write test sets up the Arrow memory pool the Parquet writer allocates from, so it also passes when run alone (CI review bot finding on this PR). 2. `[refactor](be) Take both paths of every Iceberg file from one output location`: - The new `IcebergOutputLocation` (header-only) holds one directory in both forms and gives the write path and the commit path of a file name, and of a partition subdirectory. - The data writer (`VIcebergTableWriter` / `VIcebergPartitionWriter`), the delete file writer and the Puffin path all take their paths from it. `VIcebergDeleteFileWriter` now reports the commit path itself instead of its write path. - A table or delete sink without both location fields is rejected in `open()` / `init_properties()` instead of committing a write path or a relative path. The FE of every branch (3.1, 4.0, 4.1, 4.2, master) sets both fields on every Iceberg sink. - Directories join file names with exactly one `/`. 3. `[improvement](fe) Reject Iceberg commit data reported outside the sink's location`: - `planWrite` registers on the transaction the raw location(s) each sink it builds carries, read back from the built sink. - `IcebergConnectorTransaction.addCommitData`, where every report from every writer enters FE, rejects a file that is not under a registered location, or has no path. FE then answers the report as not accepted, and BE deletes the files under the paths it wrote them at and cancels the query, so a wrong path fails the statement and leaves no orphan files. - Thrift comments state the contract, including that "the same directory" can differ in more than the scheme (Azure moves the account out of the authority). Comments only, no protocol change. 4. `[test](regression) Check that every Iceberg write records its files under the table location`: - The new `test_iceberg_write_file_locations` runs INSERT, INSERT OVERWRITE PARTITION, branch INSERT, DELETE, UPDATE, MERGE and `rewrite_data_files` on a v2 table, and INSERT, DELETE, UPDATE and MERGE on a v3 table. After each operation every file in `$all_files` must start with the table's LOCATION plus `/data/`. A new write operation belongs in this suite too. - `test_iceberg_update_delete_basic` checks the prefix of its delete files as well, and the stray `.out.org` is removed. | Scenario | Before | After | | --- | --- | --- | | DELETE / UPDATE / MERGE on a v2 table at `oss://`, `s3a://`, `abfss://`, ... | position delete files recorded as `s3://...` | recorded under the table's location | | The same on a v3 table | deletion vector (Puffin) files recorded as `s3://...` | recorded under the table's location | | A BE writer reports a file outside the location its sink carries | committed as reported | the statement fails; BE deletes the file | | A sink plan without `original_output_path` / `table_location` (no supported FE sends one) | data files committed as `/p=1/x.parquet`, delete files as `s3://...` | rejected before any file is written | | `write.data.path` ending with `/` | data files at `.../data//p=1/x.parquet` | `.../data/p=1/x.parquet`, as Iceberg's own location provider places them | Delete files that an older version already committed with `s3://` keep that address until they are rewritten or expired. Upgrade BE before FE, as usual. An upgraded FE rejects the `s3://` delete file paths that a BE without this fix reports, so until that BE is upgraded, DELETE / UPDATE / MERGE on a table whose location FE normalizes fails instead of committing the wrong path. #### Classes, and how they call each other - `IcebergWritePlanProvider.planWrite` (FE) builds the sink and registers the raw location(s) it carries with the transaction (`commitLocationsOf`). - `IcebergConnectorTransaction.addCommitData` (FE) accepts a reported file only under a registered location; `IcebergWriterHelper` then turns the fragments into `DataFile` / `DeleteFile`. - `IcebergOutputLocation` (BE, new) is one directory in its write form and its commit form. - `VIcebergTableWriter` builds the data location in `open()` and hands each `VIcebergPartitionWriter` its partition directory (`WriteInfo.location`). `VIcebergDeleteSink` writes position delete files through `VIcebergDeleteFileWriter` and deletion vectors into one Puffin file, both from its location. `VIcebergMergeSink` builds one of each. ``` FE planWrite ── sink: output_path + raw location ──────────────► BE │ addCommitLocation(raw location) IcebergOutputLocation(output_path, raw location) ▼ ├─ VIcebergTableWriter ─► VIcebergPartitionWriter data files IcebergConnectorTransaction ├─ VIcebergDeleteSink ──► VIcebergDeleteFileWriter position deletes ▲ addCommitData: file_path under a registered │ └─► Puffin writer deletion vectors │ location, else reject the report └─ VIcebergMergeSink ──► both of the above └──────────── TIcebergCommitData.file_path = commit path ◄───────┘ ``` Not changed: Thrift fields (comments only), the scan side, and how FE normalizes locations.
… writes to a reused vault id Issue Number: None Related PR: apache#69605 Problem Summary: Two regressions in how a BE applies a change of backend (S3 family to Azure or back), plus the review's comment and test follow-ups. A cold-storage resource: FE allows `ALTER RESOURCE` to change the provider even under a storage policy, and `S3FileSystem::refresh` built a new file system for it. A remote rowset resolves its file system once and keeps it, so rowsets bound before the change stayed on the old holder: after a revert or a key rotation they kept failing with the old backend or the old key until BE restart, while FE and the BE log reported success. `refresh` now keeps the file system while the bucket and prefix stay, and rebinds its holder to a client built from the new configuration (`ObjClientHolder::rebind`), so every rowset follows, as before the refactor. It builds a new file system only when the bucket or prefix moves, which FE only allows without a storage policy. Every backend change logs a WARNING with the resource id and both configurations, masked. A storage vault: a vault's backend cannot be altered, but `drop_hdfs_vault` can drop an object vault and the meta service can hand its id to the next vault. The vault refresh only reset the holder, which refuses another backend, so loads for the new vault kept writing into the dropped vault's bucket; before the refactor they failed. `S3FileSystem::refresh_vault` now takes the holder out of service in that case (`ObjClientHolder::invalidate`). Every user shares the holder, including `PackedFileManager`, so all of them fail with their existing "no client" errors until a BE restart builds the new vault's file system. The invalidation is permanent: `reset`, `rebind` and `init` return its reason, and `_publish` checks it under the lock before the generation, so a refresh carrying an older configuration cannot bring the client back. The shared SDK client is left alone, since other holders may use it. Because the client can now become null at runtime: `S3FileWriter::_complete` takes it after waiting for the uploads, so that it no longer returns with upload callbacks still running and sees an invalidation during the wait, and `generate_presigned_url` checks it before its OSS branch, which would build a client of its own. Limits, accepted here: a call already sent cannot be recalled, so a PutObject that took the client may still succeed. Until the next vault sync (60s by default) a BE cannot know the id was reused. A reused id on the same backend or between HDFS and S3 is not detected. These need the fix in the meta service, which is handled separately. Review follow-ups: the comments on why Recycler does not check the Azure key pair now give the real reason (`S3Conf::from_obj_store_info` loads the pair complete or not at all; the meta service only requires it when an entry is created). New tests pin which value becomes the Azure account and which the key, in `AzureAuthFactory`, `AzureClientProvider` and Recycler's `S3Accessor`, without an account. `ObjClientFactoryTest` cases that count builds use unique endpoints, so they can be repeated in one process. None - Test: Unit Test (holder rebind across S3, Azure and back with a kept reader, vendor change inside the S3 family, invalidation and overlapping reset, vault refresh, presigned URL, writer timing with sync points, Azure credential wiring in BE and Recycler; BE UT and cloud UT in CI) - Behavior changed: Yes. See the PR description, items 3 and 5. - Does this need documentation: No
Contributor
Author
|
run buildall |
…torage-client # Conflicts: # be/src/exec/sink/viceberg_delete_sink.cpp # be/src/exec/sink/writer/iceberg/viceberg_partition_writer.cpp # fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergWritePlanProviderTest.java # gensrc/thrift/DataSinks.thrift # gensrc/thrift/PlanNodes.thrift
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: `ObjStorageFileSystem::refresh` applies a refreshed cold-storage resource and can return a new file system, while `refresh_vault` applies a refreshed storage vault and never replaces the file system. The bare name read like a general version that a vault could use too, so it is now `refresh_resource`. No behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (rename; existing unit tests cover it, BE UT in CI) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29073 ms |
Contributor
TPC-DS: Total hot run time: 151627 ms |
Contributor
ClickBench: Total hot run time: 24.32 s |
…torage-client # Conflicts: # be/src/exec/sink/writer/iceberg/viceberg_delete_file_writer.cpp # be/src/exec/sink/writer/iceberg/viceberg_partition_writer.cpp # be/src/exec/sink/writer/vhive_partition_writer.cpp
Contributor
Author
|
run buildall |
xylaaaaa
added a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 9, 2026
…mment ### What problem does this PR solve? Issue Number: None Related PR: apache#69605, apache#69082 Problem Summary: The bulk rename of S3ClientFactory to AwsSdkManager also rewrote a comment in obj_storage_config.cpp that names the function the config checks came from, so it read "the former AwsSdkManager::create". That function was S3ClientFactory::create; AwsSdkManager never had a create method. Restore the original name. A scan of comments that name a method of AwsSdkManager or the ObjStorage* classes found no other method that does not exist. ### Release note None ### Check List (For Author) - Test: No need to test (comment only) - Behavior changed: No - Does this need documentation: No
added 2 commits
November 9, 2026 14:44
### What problem does this PR solve? Related PR: apache#69082, apache#69605 Problem Summary: Keep the integration branch aligned with the split BE refactor. Include the bucket supplied by the legacy configuration sources in the S3 client cache key and preserve it through refresh. This keeps separate connection pools for otherwise identical configurations that name different buckets. ### Release note None ### Check List (For Author) - Test: FE and ASAN BE builds passed. A local two-bucket S3 regression passed with cold and warm caches. BE logs confirmed two client builds, and SQL and an independent S3 client verified the exported data. Header hygiene, formatting and clang-tidy passed. - Behavior changed: No (preserves the pre-refactor S3 cache behavior) - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#69082 Problem Summary: Public URL generation assumed an S3 client whenever an endpoint matched the OSS private endpoint suffix. A custom Azure endpoint with that suffix would fail before signing. Rewrite the endpoint in a copy of the configured backend so its own client signs the public URL. ### Release note None ### Check List (For Author) - Test: FE and ASAN BE builds passed. The file system and regression test compiled. The macOS BE UT target stopped at the pre-existing std::min type mismatch in binary_column_extract_iterator_test.cpp:74 from apache#70212. Header hygiene, clang-format 16 and clang-tidy passed. - Behavior changed: No (restores pre-refactor public URL generation). - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #67470
Problem Summary:
Doris reads Azure Blob natively from BE, but only with SharedKey, and the whole path is shaped around S3:
provider=azure, the account inAWS_ACCESS_KEYand the key inAWS_SECRET_KEY. The map cannot carry a SAS token or Entra ID client credentials, and BE infers the authentication method from which keys are present.FILE_S3reader cannot use.S3ClientConf(Azure reusesak/sk/bucketas account/key/container, next to S3-only fields such as region and role), oneS3ClientFactorythat builds both SDK clients and owns one shared cache, and anObjClientHolderthat merges a new conf field by field and reads its conf without the lock.This PR adds native SAS and OAuth2 (client credentials) access for Azure end to end, together with the BE refactor it builds on. It is kept as one PR so the whole path can be tested end to end; it can be split into the BE refactor, the protocol/BE part and the FE part for merging.
1. BE object storage refactor
S3ConfandS3ClientConfare gone.io::ObjClientConfigholds shared connection options, a client usage (internal storage / external access) and exactly one backend,std::variant<S3ClientConfig, AzureClientConfig>; the Azure authentication is a variant as well (SharedKey, SAS, client secret).io::ObjStorageConfig(bucket, prefix, client config) replacesS3Conf.ObjStorageConfigResolveris the only conversion point:from_properties()(FEAWS_*properties, formerlyconvert_properties_to_s3_conf),from_storage_param()(cold-storage resources) andfrom_object_store_info()(storage vaults), formerlyS3Conf::get_s3_conf, plusfrom_azure_client_config()for the typed Azure input. The flat field mapping and validation are ported unchanged.merge_update()merges a refreshed resource or vault config with the flat-merge field rules; a different backend is rejected as a different storage target.ObjClientFactoryvalidates every request with the rules of its backend, then dispatches throughstd::visit(a new backend without a branch fails to compile).S3ClientProviderandAzureClientProvidereach build and cache their clients outside the cache lock; one shared function applies the BE rate limiter. The Azure cache is bounded byazure_client_cache_capacity(default 256, mutable): expired SAS clients go first, then the least recently used; the capacity lives in the cache and changes take effect at once.S3ClientFactoryis renamed toAwsSdkManager: it no longer builds clients and keeps only the AWS SDK lifetime and AWS helpers (client configuration, credentials, CA path), which Kinesis also uses.FileWriterOptions::used_by_s3_committeris renamed todeferred_completion, andObjStorageCapabilities::deferred_completionsays what the coordinator must support (ANY_COORDINATORfor S3 multipart,NAMESPACED_BLOCK_IDSfor Azure blocks).VHivePartitionWriterchecks it against the coordinator'ssupports_deferred_azure_multipartinstead of comparing providers.ObjClientHolder(nowio/obj_storage/obj_client_holder.{h,cpp}) owns an immutableObjClientState(config, client, generation) and replaces it as a whole. A reset merged onto a state that another reset already replaced returnsABORTEDinstead of overwriting the newer update.ObjStorageUrireplacesS3URI:parse_s3()keeps the S3 rules (and takes container and key from native Azure locations),parse_azure()adds the Azure rules (below).ObjStorageFileSystem,ObjStorageFileReader,ObjStorageFileWriter(obj_storage_file_*.{h,cpp}),obj_storage_file_buffer_pool,display_path(),FileSystemType::OBJECT_STORAGE(same value). Only identifiers and comments changed: debug and sync point names are kept because regression tests trigger them by name. AWS residue left the common layer (the RFC 3986 encoding flag is set once at SDK init; reader 429 handling no longer uses AWS types). Internal metric variables are renamed toobj_storage_*; exposed metric names and config keys are kept for compatibility.2. FE-to-BE protocol (gensrc)
TAzureClientConfig { endpoint, account_name, container, credential }withunion TAzureCredential { shared_key, sas, client_secret }, carried by the new optional fieldTFileScanRangeParams.azure_client_config(id 42; ids 41/40 are the last ones on master and branch-4.2/4.1, branch-4.0/3.1 end lower, so 42 is unused everywhere).containeris optional: BE takes it from each file location and rejects a configured container that differs.The write sinks carry the same struct as a new optional
azure_client_config:TIcebergTableSink(21),TIcebergDeleteSink(16),TIcebergMergeSink(26),THiveTableSink(15). Each id is one past the last id on master, and unused on branch-4.2/4.1/4.0/3.1.The other entries that reach object storage carry it too:
TParquetMetadataParams(7,parquet_meta),TResultFileSinkOptions(24, OUTFILE / EXPORT data and success files) andTTVFTableSink(17, TVF insert); TVF schema inference already received it and BE now copies it infetch_file_schema. These ids are unused on every branch as well. Without the typed config BE falls back to the S3-style keys, which can express only SharedKey.3. BE
AzureAuthFactorybuilds SAS clients (token appended to the container URL) and client secret clients (Azure::Identity::ClientSecretCredential, sharing the blob transport and CA bundle; the SDK requests and refreshes the access token).AzureConfigCodecdecodes the Thrift config (exactly one credential);AzureConfigValidatorchecks the fields of the selected method and the account name against standard Azure hosts;AzureCredentialValidatorparses the SASseexpiry and rejects an expired SAS.AzureClientProviderruns it on every client request, i.e. each reader open, never per range read.io::ObjStorageUriparses a location by the Azure rules:abfs[s]://container@acct.dfs...,wasb[s]://container@acct.blob...,http(s)://acct.blob.../container/keyand the legacys3://container/keyall become account, container and key, and the original location is kept untouched. The resolver rejects a location whose account differs from the one requests go to (account_name, or the account the endpoint names).FileFactoryreader,create_fsandcreate_file_writerresolve aFILE_S3location through one function: typed Azure access when FE sends it, theAWS_*properties otherwise. The scan readers copy the field fromTFileScanRangeParamsthrough one helper,io::azure_client_config_of(); the Iceberg table/delete/merge writers, the rewritable-delete reader and the Hive writer pass the sink's field toFileFactory.VIcebergDeleteSinkused to report position delete and puffin files underoutput_path, the normalized form (s3://b/db/t/datafor anoss://or Azure table), so Iceberg metadata recorded the internals3://address. It now commits them undertable_location, the raw data location, which every FE version sets to the same directory asoutput_path. Data files were already committed underoriginal_output_path.4. FE
azure.sas_token(sensitive) withazure.auth_type = SAS; validation requires the token and an endpoint or account name. FE listing (AzureObjStorage) and the Hadoop ABFS map (fixed SAS token) support it too.AzureFileSystemProperties.toMap()keeps its S3-style keys for old BEs and also writes neutralAZURE_*keys (AzureBackendPropertyKeysin fe-filesystem-api). SAS adds no legacy credential, so an old BE is never handed a wrong one. OAuth2 resolves the tenant as the FE SDK client already does and derives the authority host fromazure.oauth2_server_uri; the FE SDK client now uses that authority host as well.AzureThriftAdapter(fe-core) turns the neutral keys intoTAzureClientConfig, andFileQueryScanNodesets it forFILE_S3ranges. TVF scans and plugin-driven catalogs both build their location properties from the provider map, so both are covered. The TVF fetch-schema request, which BE serves before any scan, carries it too.azure.auth_type = OAuth2is only accepted for an Iceberg REST catalog is removed: it existed because BE could only use OAuth2 through the Iceberg REST / OneLake Hadoop path.adls.sas-token.<account host>andadls.sas-token-expires-at-ms.<account host>with each table. These keys used to be dropped by the vended-credential filter.IcebergAdlsVendedCredentialsnow turns them into Azure SAS properties (account and Blob endpoint from the host, token, expiry), so Iceberg scans and writes, which already overlay the vended properties, send them to BE asTAzureClientConfig. The expiry travels asTAzureSas.expires_at_ms. One storage account per table: tokens for several accounts are rejected with a message naming them.PluginDrivenTableSinkbinds the typed config onto Iceberg and Hive sinks (AzureThriftAdapter.bindToWriteSink), from the backend properties the connector already put in the sink, so connectors are unchanged.DefaultConnectorContext(Iceberg, Hive, Paimon, Hudi scan ranges and write paths) ends up in a request that carries typed Azure access, so it now usesLocationPath.ofAdaptersForNativeAccess: Azure locations are validated and kept asabfss://c@acct.dfs.core.windows.net/...instead of being rewritten tos3://c/..., which dropped the account; other storages are normalized as before. BE can then check every file's account, and Hive commits record the native location in HMS. Requests without typed access (Broker Load, Export, Outfile, backup, TVFs, streaming offsets) still get thes3://form, which is all their BE entries parse. Paimon and Hudi JNI readers use raw paths and are unaffected; the FE Azure filesystem accepts the native forms for Hive multipart completion.FileQueryScanNodebinds the typed config even when a JNI scan already set the scan properties, which fixes SAS and OAuth2 reads of Icebergposition_deletes; the Iceberg plugins package the Azure FileIO dependencies.Scope of this PR: reads of external tables and TVFs, and Iceberg/Hive writes. Outfile/Export, TVF insert, Broker Load, Resource and Storage Vault still use the S3-style properties, so SAS and OAuth2 are not available there yet.
Rolling upgrade: SharedKey reads keep working on old BEs through the S3-style keys. SAS and OAuth2 on
FILE_S3require every BE to be upgraded before they are used. Connector scans and writes send nativeabfss://locations, which old BEs cannot parse, so BE must be upgraded before FE (the standard order). After the upgrade, the BE file cache keys and split affinity of Azure files change once, because their path strings change.Outbound requests (SSRF surface): with OAuth2, BE now requests access tokens from the authority host derived from
azure.oauth2_server_uri, in addition to the storage endpoint it already contacted. Both values come from catalog or TVF properties, so reaching them requires the privilege to create or alter the catalog, or to run the TVF.Release note
Azure storage accessed through TVFs and external catalogs, including Iceberg and Hive writes, supports SAS tokens (
azure.auth_type = SAS,azure.sas_token) and native BE OAuth2 client credentials, in addition to SharedKey. Iceberg REST catalogs that vend Azure SAS (such as Databricks Unity Catalog) can read and write their Azure data through the vended SAS. Iceberg position delete files are committed with the table's own location instead of the internals3://form. Upgrade all BEs before FE and before using SAS or OAuth2.Check List (For Author)
Test
obj_client_factory_test.cpp(resolver mapping from resource params, properties and vaults, backend-specific identity, masking, provider caching, validation, bounded Azure cache, holder snapshots, concurrent reset conflict, reset merge and backend change, expired SAS rejected before any client is built),obj_client_cache_test.cpp(LRU, expiry-first eviction, capacity changes racing with publishes),obj_storage_uri_test.cpp(the formerS3URIcases),azure_typed_access_test.cpp(typedcreate_fs/create_file_writer)azure_config_test.cpp(codec, validator, SAS expiry parsing, resolver entry,ObjStorageUrilocation forms, account checks) andviceberg_delete_sink_test.cpp(delete/puffin commit path undertable_location)AzureFileSystemPropertiesTest(native keys for SharedKey/SAS/OAuth2, SAS validation and Hadoop config, sensitive SAS key),IcebergAdlsVendedCredentialsTest(vended SAS translation, single-account rule),AzureThriftAdapterTest(including binding onto Iceberg/Hive sinks and SAS expiry) andStorageAdapterAzureNativeTest(OAuth2 outside Iceberg REST and SAS, throughStorageAdapter)build-support/check-build-hygiene.shpassedBehavior changed:
FILE_S3scans and Iceberg/Hive writes carryTAzureClientConfigand BE uses it; Iceberg writes go to the native Azure location; Iceberg delete files are committed undertable_location; new SAS auth type; OAuth2 is accepted for TVFs and all catalogs, not only Iceberg REST; a reset racing with another reset on the same holder returnsABORTED; resetting a holder to another backend is rejected; flat Azure configs no longer need a placeholder region;FileSystemTypeprintsOBJECT_STORAGE; new BE configazure_client_cache_capacity.Does this need documentation?
azure.auth_type = SAS,azure.sas_tokenand the upgrade order; not included yet.Check List (For Reviewer who merge this PR)