morningman commented on PR #69605: URL: https://github.com/apache/doris/pull/69605#issuecomment-6546301394
<!-- doris-repo-review:report:begin --> Review at head `af4964b9ceb3`. No build or test was run locally; see "Coverage and limits" at the end. This is not an approval. # Code Review — PR #69605: [refactor](be) Build object storage clients from typed, per-backend configuration | | | |---|---| | PR | https://github.com/apache/doris/pull/69605 (state: open) | | PR head | `af4964b9ceb3e331be2c6c0ab62d7a9a162673f0` on `refactor/be-obj-client-config` of `xylaaaaa/doris` | | Diff range | `b8dc326111ff1c01e920ede7ea82714d207d8fc5...af4964b9ceb3e331be2c6c0ab62d7a9a162673f0` (three-dot, merge base `b8dc326111ff1c01e920ede7ea82714d207d8fc5`) | | Scope | 60 files, +3071/-1626 lines, 4 commits | | Verdict | **REQUEST_CHANGES** | | Rounds | 3 of max 3; converged (verdict stable since round 1, blocking set stable since round 2) | | Builds on | `719de6a`, reviewed 2026-10-26 | ## Summary This PR replaces the flat `S3Conf` / `S3ClientConf` with typed per-backend configuration and moves client construction into per-backend providers. The port is faithful: S3-family validation matches the merge base in 2048 of 2048 modelled cells, URI parsing in 559870 inputs, and client construction option by option. COMPILE, BE UT and the regression gates passed at this head on 2026-10-26 and 2026-10-27. The Blocker of the previous review (`secret_key_of`) is fixed, and so is its Minor on the Azure region wording. It cannot merge yet, for three reasons. A Thrift value the merge base rejected with a Status now terminates the BE (F-01). Merged into current master, the BE UT build fails (F-02). The required `Check for secrets` gate is red and needs a history rewrite (F-03); F-02 and F-03 are one rebase. The two Majors are consequences of declared behaviour changes that the PR does not declare: item 3 lets cold data land on a retired target (F-04), and item 1 changes the scope of the connection limit (F-05). ## Findings at a glance | ID | Severity | Regression | Finding | |---|---|---|---| | F-01 | Blocker | yes | A native credential on an Azure resource now terminates the BE instead of returning InvalidArgument | | F-02 | Blocker | no | Merged into current master, the BE UT build fails | | F-03 | Blocker | yes | The required check `Check for secrets` is red, and a follow-up commit cannot clear it | | F-04 | Major | yes | FE accepts a provider-family ALTER that running BEs ignore, so cold data goes to the retired target | | F-05 | Major | yes | `s3.connection.maximum` changed scope from per bucket to per client identity, and the PR does not say so | | F-06 | Minor | no | A failed vault sync logs the wrong Status | | F-07 | Minor | no | The `reset` comment and item 2 claim more than the generation check gives | | F-08 | Minor | yes | Azure lost the role_arn exemption, while the code and PR body say validation is unchanged | | F-09 | Minor | no | The resolver header claims to be the single place for every input source | | F-10 | Minor | no | Nothing pins the RFC 3986 flag, which now rests on one line after `InitAPI` | | F-11 | Minor | no | Nothing pins the BE rate limiter on Azure clients | | F-12 | Minor | no | The Azure arm of `merge_obj_client_config` has no test | | F-13 | Minor | no | GCP credentials are tested on one of the three sources the PR claims | | F-14 | Nit | no | Stale includes and an unused export remain after the move | | F-15 | Nit | no | The new 429 comment states something false about S3 | | F-16 | Nit | no | The "never a mix" example uses two calls that take two snapshots | | F-17 | Nit | no | The PR body still names `merge_update` | | F-18 | Nit | no | The microbench reader sets a key nothing reads | | F-19 | Nit | no | Neither `empty endpoint` branch has a test | | F-20 | Nit | no | The `#` half of the rewritten URI strip is untested | ## Findings ### F-01 · A native credential on an Azure resource now terminates the BE instead of returning InvalidArgument - **Severity**: Blocker - **Category**: functional-bug (BE crash on cold-storage push) - **Regression**: yes - Regression evidence: `git show b8dc326111ff:be/src/util/s3_util.cpp` lines 97-133 and 263 return InvalidArgument for this input, and the base `update_s3_resource` logs it and continues. - **Where**: [`be/src/io/obj_storage/obj_storage_config_resolver.cpp:120-124`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L120-L124) ```cpp std::variant<S3ClientConfig, AzureClientConfig> flat_backend(const FlatClientFields& fields) { if (fields.provider == ObjStorageProvider::AZURE) { // Provider-native credentials are GCP only. FE properties reject one for Azure in // validate_property_rules, and FE and the meta service never store one for Azure. DORIS_CHECK(std::holds_alternative<std::monostate>(fields.credential)); ``` **What is wrong.** `from_storage_param` and `from_object_store_info` became infallible, so a case that `S3ClientFactory::create` used to reject is now a `DORIS_CHECK`. The value crosses a process boundary, so it is input, not an invariant, and the claim in the comment that FE never stores one for Azure is false. **Why it happens.** FE validates one spelling and sends another. `GcsAuthResolver` reads `provider` case-insensitively and takes the first match ([`GcsAuthResolver.java:52`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-filesystem/fe-filesystem-gcs-common/src/main/java/org/apache/doris/filesystem/gcs/auth/GcsAuthResolver.java#L52), [`GcsAuthResolver.java:166-172`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-filesystem/fe-filesystem-gcs-common/src/main/java/org/apache/doris/filesystem/gcs/auth/GcsAuthResolver.java#L166-L172)), while `S3ThriftAdapter` reads the exact key ([`S3ThriftAdapter.java:75-76`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/S3ThriftAdapter.java#L75-L76)). A resource holding both `provider=azure` and `Provider=GCP` passes the gate "Native GCP authentication requires provider=GCP" at [`S3Resou rce.java:283`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java#L283), and is then sent as AZURE with a GCP credential ([`S3ThriftAdapter.java:120-121`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/S3ThriftAdapter.java#L120-L121)), because `applyTo` keeps a provider that is already set ([`ObjectStorageCredentialAdapter.java:62-64`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/ObjectStorageCredentialAdapter.java#L62-L64)). On the BE, [`task_worker_pool.cpp:1726`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L1726) or [`task_worker_pool.cpp:1736`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L1736) runs `from_storage_param`, which converts the credential at [`obj_storage_config_resolver.cpp:360-362`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L360-L362) and fails the check. `doris_check_fail` throws ([`check.cpp:25-27`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/common/check.cpp#L25-L27)), and nothing catches it: the pool lambda at [`task_worker_pool.cpp:561-564`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L561-L564) has no try, `be/src/util/threadpool.cpp` has no catch, a nd `be/src` has no `set_terminate`. **When it bites.** A user with ADMIN on a non-cloud cluster: ```sql CREATE RESOURCE r PROPERTIES ("type"="s3", "provider"="GCP", "s3.endpoint"="storage.googleapis.com", "s3.region"="us", "s3.bucket"="b", "s3.root.path"="p", "gs.credential_provider_type"="COMPUTE_ENGINE", "s3_validity_check"="false"); ALTER RESOURCE r PROPERTIES ("provider"="azure", "Provider"="GCP"); ``` FE pushes every S3 resource on each tablet report, referenced or not ([`ReportHandler.java:408-416`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/master/ReportHandler.java#L408-L416)). So every BE aborts within one report interval, and again after a restart, until an operator corrects the resource on FE. At the merge base the same input is one WARNING per report. Two limits on the trigger. With `provider` spelled once, the FE gate holds. The lookup order comes from the hash table: `Provider` (bucket 15) precedes `provider` (bucket 21) in a 32-bucket HashMap, which is what a resource with 12 to 23 properties has; in a 16 or 64-bucket table the gate fires. This was traced statically and the lookup order checked with a small Java program; the statements were not run on a cluster. The meta service path is closed: every writer validates ([`meta_service_resource.cpp:809`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/cloud/src/meta-service/meta_service_resource.cpp#L809), [`meta_service_resource.cpp:1258`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/cloud/src/meta-service/meta_service_resource.cpp#L1258), [`meta_service_resource.cpp:1354`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/cloud/src/meta-service/meta_service_resource.cpp#L1354)). **Severity rationale.** The trigger is a contradictory statement by an administrator. The consequence is every non-cloud BE down in a loop, and severity follows the consequence. `be/src/common/AGENTS.md` also limits `DORIS_CHECK` to invariants. **Suggested fix.** Treat it as an input error, which already has a Status path: ```diff - static ObjStorageConfig from_storage_param(const TS3StorageParam& param); - static ObjStorageConfig from_object_store_info(const cloud::ObjectStoreInfoPB& info); + static Result<ObjStorageConfig> from_storage_param(const TS3StorageParam& param); + static Result<ObjStorageConfig> from_object_store_info(const cloud::ObjectStoreInfoPB& info); ``` Apply the Azure rule of `validate_property_rules` on all three sources, remove the `DORIS_CHECK`, and fix the comment. `update_s3_resource` already holds a Status. At [`cloud_meta_mgr.cpp:1913-1917`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/cloud/cloud_meta_mgr.cpp#L1913-L1917) keep the outcome per vault, so one bad vault does not lose the whole sync. Add a resolver test for Azure plus a credential on both metadata sources. Separately from this PR, FE should reject two spellings of `provider`, or read it one way in `S3ThriftAdapter` and `GcsAuthResolver`. ### F-02 · Merged into current master, the BE UT build fails - **Severity**: Blocker - **Category**: functional-bug (test build) - **Regression**: no - Regression evidence: PR head and merge base both compile. This comes from commits the PR branch has never seen, so it is a semantic conflict with the target branch. - **Where**: [`be/src/io/obj_storage/obj_client_holder.h:53`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.h#L53) and [`be/src/io/obj_storage/obj_client_factory.h:41-42`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_factory.h#L41-L42), the replacements those tests need. The broken lines exist only on master. ```cpp explicit ObjClientHolder(ObjClientConfig config); ``` **What is wrong.** The PR removes `S3ClientConf`, `S3ClientFactory::set_client_creator_for_test`, `clear_client_creator_for_test` and `ObjClientHolder::_client`. Master gained tests that use them after the PR base. GitHub shows no conflict. **Why it happens.** The PR base is from 2026-10-26, and master tip `8e10b235cf03` is 131 commits ahead. `git merge-tree --write-tree af4964b9ceb3 8e10b235cf03` is clean. In the merged tree these lines remain: - `be/test/format_v2/delimited_text/csv_reader_test.cpp` lines 604, 614-615, from #69716 - `be/test/io/fs/s3_file_reader_test.cpp` lines 75-76, 116-117, 163-164, a new file from #67292 [`CMakeLists.txt:24-45`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/CMakeLists.txt#L24-L45) collects both by glob. No production file is affected, and the build hygiene gate passes on the merged tree. **When it bites.** Every TeamCity gate last ran on a merge ref from 2026-10-26 or 2026-10-27, and `.asf.yaml` sets `strict: false`, so those greens do not cover the two files. A merge on them breaks BE UT on master for everyone. The rewrite that F-03 needs re-runs CI, so BE UT goes red in the same push. **Suggested fix.** Rebase on master and migrate both files: ```diff --- a/be/test/format_v2/delimited_text/csv_reader_test.cpp +++ b/be/test/format_v2/delimited_text/csv_reader_test.cpp - S3ClientFactory::instance().clear_client_creator_for_test(); + io::ObjClientFactory::instance().set_client_creator_for_test(nullptr); - S3ClientFactory::instance().set_client_creator_for_test( - [client](const S3ClientConf&) { return client; }); + io::ObjClientFactory::instance().set_client_creator_for_test( + [client](const io::ObjClientConfig&) { return client; }); --- a/be/test/io/fs/s3_file_reader_test.cpp +++ b/be/test/io/fs/s3_file_reader_test.cpp - auto holder = std::make_shared<ObjClientHolder>(S3ClientConf {}); - holder->_client = std::make_shared<S3ObjStorageClient>(client); + auto holder = std::make_shared<ObjClientHolder>(ObjClientConfig {}); + holder->set_client_for_test(std::make_shared<S3ObjStorageClient>(client)); ``` `csv_reader_test.cpp` also needs `#include "io/obj_storage/obj_client_factory.h"`. The holder change applies at all three sites. ### F-03 · The required check `Check for secrets` is red, and a follow-up commit cannot clear it - **Severity**: Blocker - **Category**: functional-bug (CI gate) - **Regression**: yes - Regression evidence: the literal does not exist at the merge base. Commit 719de6ac81f adds it. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:164`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_client_factory_test.cpp#L164) ```cpp EXPECT_EQ(std::get<AzureSharedKeyConfig>(azure.auth).account_key, "YWNjb3VudC1rZXk="); ``` **What is wrong.** gitleaks reports one `generic-api-key` finding here. The value is a fixture that decodes to `account-key`, so nothing leaked, but the gate is red. **Why it happens.** `.github/workflows/gitleaks-pr-check.yml` scans every commit in `--no-merges ${BASE_SHA}..${HEAD_SHA}`, and the finding is in commit 719de6ac81f. Actions run 40338187924, job 123940578096, at this head: 4 commits scanned, 1 leak. **When it bites.** Now. `Check for secrets` is a required context for master in `.asf.yaml`, and the PR shows `mergeable_state: blocked`. Removing the line in a new commit leaves the old commit in the scanned range, so the check stays red. **Suggested fix.** Change the test inside commit 719de6ac81f (amend or squash, then force-push), and compare with the input rather than a literal. No allowlist entry is needed. ```diff - auto config = resolve(make_azure_param("resolve-azure.example.com")); + auto param = make_azure_param("resolve-azure.example.com"); + auto config = resolve(param); - EXPECT_EQ(std::get<AzureSharedKeyConfig>(azure.auth).account_key, "YWNjb3VudC1rZXk="); + EXPECT_EQ(std::get<AzureSharedKeyConfig>(azure.auth).account_key, param.sk); ``` ### F-04 · FE accepts a provider-family ALTER that running BEs ignore, so cold data goes to the retired target - **Severity**: Major - **Category**: functional-loss (cold-storage ALTER RESOURCE) - **Regression**: yes - Regression evidence: `git show b8dc326111ff:be/src/io/fs/s3_file_system.cpp` lines 83-123 apply the provider in `reset`. Item 3 declares the rejection. It does not declare that the ALTER still reports success, that the BE keeps the old target, or that a restart moves it. - **Where**: [`be/src/agent/task_worker_pool.cpp:1736-1737`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L1736-L1737) ```cpp auto config = io::ObjStorageConfigResolver::from_storage_param(param.s3_storage_param); st = client->reset(config.client); ``` **What is wrong.** The comment at [`obj_storage_config.h:156-160`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L156-L160) says a different backend "needs a new binding rather than a reset", but no caller builds one. FE reports the ALTER as successful, and a running BE silently stays on the old backend. **Why it happens.** FE freezes endpoint, region, root path and bucket only under a POLICY reference ([`S3Resource.java:250-251`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java#L250-L251)), and it never freezes the provider. On the BE, [`obj_storage_config.cpp:225-230`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L225-L230) rejects the merge, and [`task_worker_pool.cpp:1741-1747`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L1741-L1747) logs a WARNING with no resource id and skips `put_storage_resource`. This task type is not reported back ([`task_worker_pool.cpp:128-132`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L128-L132)), so FE re-pushes on every report and never sees the failure. Cooldown then uses the registered filesystem whatever its version ([`storage_policy.cpp:43-50`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/storage/storage_policy.cpp#L43-L50)). **When it bites.** Non-cloud, with nothing contradictory in the input: | Step | Merge base | This head | |---|---|---| | 1. S3 resource r, no policy, on E1/B1 with keys K1 | BE builds the filesystem | Same | | 2. `ALTER RESOURCE r` sets `provider=azure`, E2, B2, K2; FE reports success | | Same | | 3. Push reaches the BE | `reset` applies provider, keys and bucket, keeps E1, records the version | Rejected. Stays on E1/B1/K1, version stale, one WARNING per report | | 4. `CREATE STORAGE POLICY` on r, a table uses it | Endpoint and bucket are now frozen | Same | | 5. Cooldown | Azure SDK against S3 endpoint E1 fails, the rowset stays local, the failure is logged | Upload to E1/B1 succeeds ([`tablet.cpp:2176`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/storage/tablet/tablet.cpp#L2176)), then the local rowset is deleted ([`tablet.cpp:2203`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/storage/tablet/tablet.cpp#L2203)) | | 6. BE restart | Fresh filesystem on E2/B2, nothing misplaced | The `!existed_fs` branch builds Azure on E2/B2 ([`task_worker_pool.cpp:1723-1727`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/task_worker_pool.cpp#L1723-L1727)). Cooled rowsets are not there, queries on them fail, and ALTER back is refused by the freeze | Key rotation is a second consequence. While stuck, every later push carries the same mismatching backend, so rotated keys on r are dropped while FE shows them applied. After a partial restart, BEs read and write different targets. **Severity rationale.** Rated at the floor. It was first proposed as Minor on the grounds that item 3 declares the rejection, and an independent challenge raised it. The loud failure at the merge base is what protected the data. It is not a Blocker because the data still exists in the old bucket and can be copied by hand. A related stale-target hazard already exists for a same-family endpoint or bucket ALTER, since `reset` never merges those, but there the failure stays loud unless the new keys also work on the old target. **Suggested fix.** Build the new binding that the comment describes, and name the resource in the log: ```diff auto config = io::ObjStorageConfigResolver::from_storage_param(param.s3_storage_param); - st = client->reset(config.client); - fs = std::move(existed_fs); + if (client->state()->config.backend.index() != config.client.backend.index()) { + // reset() keeps the storage target, and FE allows a provider-family ALTER on a + // resource: S3 to Azure is a new target, so publish a new filesystem for it. + auto res = io::S3FileSystem::create(std::move(config), std::to_string(param.id)); + if (res.has_value()) { + fs = std::move(res).value(); + } else { + st = std::move(res).error(); + } + } else { + st = client->reset(config.client); + fs = std::move(existed_fs); + } ``` Otherwise reject the provider-family change in `S3Resource.modifyProperties`, and state the limitation in item 3. Either way, add `param.id` and `param.name` to the WARNING. ### F-05 · `s3.connection.maximum` changed scope from per bucket to per client identity, and the PR does not say so - **Severity**: Major - **Category**: performance (connection budget across buckets) - **Regression**: yes - Regression evidence: `git show b8dc326111ff:be/src/util/s3_util.cpp` lines 276 and 296 cache on a key that holds the bucket, so each bucket had its own pool. Item 1 declares the shared client. Its next sentence reads as a claim of neutrality, and the release note says None. - **Where**: [`be/src/io/obj_storage/obj_storage_config.h:48-49`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L48-L49) ```cpp // Non-positive values keep the SDK/BE defaults, as in the legacy conf. int max_connections = -1; ``` **What is wrong.** The AWS SDK sizes one connection pool per client. With the bucket out of the S3 identity, N buckets with the same endpoint, credentials and options go from N times the limit to the limit, per BE. "Building an S3 client never used the bucket" is true of construction and says nothing about what a live client owns. **Why it happens.** In aws-sdk-cpp 1.11.221, CurlHttpClient.cpp lines 572-573 pass `clientConfig.maxConnections` to the one `CurlHandleContainer` the client owns, and `AcquireCurlHandle` blocks with no timeout when the pool is empty (CurlHandleContainer.cpp lines 37-51). The limit is applied at [`s3_client_provider.cpp:72-76`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/s3_client_provider.cpp#L72-L76). FE always sends one: 50 for native S3 ([`S3FileSystemProperties.java:69`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-filesystem/fe-filesystem-s3-base/src/main/java/org/apache/doris/filesystem/s3/S3FileSystemProperties.java#L69), [`S3FileSystemProperties.java:282`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/fe/fe-filesystem/fe-filesystem-s3-base/src/main/java/org/apache/doris/filesystem/s3/S3FileSystemProperties.java#L282)), 100 for OSS, OBS, COS, GCS, M inIO and Ozone, and 50 for cold storage ([`AgentService.thrift:133`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/gensrc/thrift/AgentService.thrift#L133)). Vaults send none and get 102400 ([`obj_storage_config_resolver.cpp:282-296`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L282-L296)), so they are unaffected. Azure keeps one client per container. **When it bites.** AWS S3, one Hive catalog with one key pair, default options, a 16-core BE, tables in two buckets: `INSERT INTO hive.db.gold_t SELECT ... FROM hive.db.bronze_t`. Scanners and the sink resolve to one client, so up to 128 scanners per scan and 64 upload threads share 50 connections, where the merge base gave each side 50. A queueing model at default sizes gives scan 499/s to 256/s and uploads 200/s to 98/s. With 512 scanners on one bucket, a 16-thread reader on another goes from 100 ms to about 1050 ms. These numbers are modelled with assumed hold times, not measured. Single-bucket deployments are unchanged. Pool wait is counted as S3 latency, so nothing tells an operator the queue is local. **Severity rationale.** Rated at the floor. There is no error, no data impact and no deadlock: the wait-for graph has no cycle, because a thread holding a handle blocks only on the network. The limit can be raised per catalog, load or resource. The bucket entered this key by accident in #35670 (2024, for Azure), so sharing is defensible and I do not ask to restore per-bucket clients. The finding is that an operator-visible limit changes scope on upgrade with no notice. **Suggested fix.** Keep the sharing and declare it. Add to item 1 and replace the release note "None": > `AWS_MAX_CONNECTIONS` (`s3.connection.maximum`, `TS3StorageParam.max_conn`) is enforced per client, per BE, so these buckets now share one limit instead of having one each. Raise it for catalogs, loads or cold-storage resources that drive several buckets at high concurrency. Storage vaults send no limit and are unaffected. ```diff + // Sizes the connection pool of one cached client on one BE. S3 clients are shared by every + // bucket of one configuration, so with 50, two buckets share 50 connections, not 50 each. // Non-positive values keep the SDK/BE defaults, as in the legacy conf. int max_connections = -1; ``` ### F-06 · A failed vault sync logs the wrong Status - **Severity**: Minor - **Category**: observability (storage vault sync) - **Regression**: no - Regression evidence: same line at the merge base. - **Where**: [`be/src/cloud/cloud_storage_engine.cpp:493`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/cloud/cloud_storage_engine.cpp#L493) ```cpp LOG(WARNING) << vault_process_error(id, vault_info, std::move(st)); ``` **What is wrong.** The line passes the outer `st` from `get_storage_vault_info`, which is OK here, instead of the visitor's `status`. **Why it happens.** `RefreshFSVaultVisitor` logs the real error itself, but `VaultCreateFSVisitor` does not, so a create failure is reported with err OK. **When it bites.** The new errors of this PR reach this line, for example "Invalid azure conf, empty account key" from [`obj_storage_config.cpp:129`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L129), and the reason is lost. **Suggested fix.** Pass `status`. ### F-07 · The `reset` comment and item 2 claim more than the generation check gives - **Severity**: Minor - **Category**: wording (concurrency guarantee) - **Regression**: no - Regression evidence: the merge base could also end on the older config; only the claim is wrong. - **Where**: [`be/src/io/obj_storage/obj_client_holder.h:62-65`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.h#L62-L65) ```cpp // The new client is built outside the lock and published only if no other reset or // init published in between. Otherwise the update is rejected rather than applied on // top of a state it was not merged with, so an older config cannot overwrite a newer // one; the caller keeps the current state and retries with its next refresh. ``` **What is wrong.** "An older config cannot overwrite a newer one" is false. The check detects overlap, not freshness. **Why it happens.** `sync_storage_vault` has four entry points, including [`cloud_storage_engine.cpp:507`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/cloud/cloud_storage_engine.cpp#L507) and [`cloud_internal_service.cpp:218-221`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/cloud/cloud_internal_service.cpp#L218-L221), and each fetches vault info before calling `reset`. **When it bites.** T1 reads vault info before an ALTER and T2 after it. T2 publishes, then T1 takes its snapshot, sees a difference and publishes the older config. In the other order, T1 publishes while T2 builds, and T2, the newer one, gets `Aborted`, which is the opposite of what item 2 says. Both heal at the next refresh, within 60 seconds by default. **Suggested fix.** Say what is guaranteed: overlapping resets cannot both win, the loser retries on the next refresh, and that refresh also heals an older config. ### F-08 · Azure lost the role_arn exemption, while the code and PR body say validation is unchanged - **Severity**: Minor - **Category**: wording (validation claim against code) - **Regression**: yes - Regression evidence: `git show b8dc326111ff:be/src/util/s3_util.cpp` lines 119-131 run the pair check only when `role_arn` is empty. - **Where**: [`be/src/io/obj_storage/obj_storage_config.cpp:126-135`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L126-L135) ```cpp Status validate_azure_auth(const AzureClientConfig& azure, const AzureSharedKeyConfig& shared_key) { // Either both are provided or both are empty, as for the flat ak/sk. if (!azure.account.empty() && shared_key.account_key.empty()) { return Status::InvalidArgument<false>("Invalid azure conf, empty account key"); } ``` **What is wrong.** The comment says "as for the flat ak/sk", and the body says validation is "ported unchanged". The flat rule exempted any config with `role_arn` set; this one does not. **Why it happens.** `role_arn` is dropped for Azure, so the pair check is unconditional. Item 4 declares only the region change. **When it bites.** Provider AZURE, `role_arn` set, exactly one of ak and sk. The merge base builds a client and every request then fails signature verification. This head fails at build. Only a cold-storage resource can reach it. **Severity rationale.** This changes control flow as well as text: `S3FileSystem::create` fails and the version is not recorded. It stays below Major because no configuration in this cell can authenticate on either side, so only the moment and the text of an inevitable failure change. An independent challenge against the floor for undeclared cells upheld this. One clause in item 4 makes it declared. **Suggested fix.** Keep the code. Reword the comment: "Unlike the flat rule there is no role_arn exemption: Azure has no role authentication, so SharedKey always needs the pair." Add the same to item 4. ### F-09 · The resolver header claims to be the single place for every input source - **Severity**: Minor - **Category**: wording (PR body and header scope) - **Regression**: no - Regression evidence: the callers keep their old mapping; only the claim is wrong. - **Where**: [`be/src/io/obj_storage/obj_storage_config_resolver.h:36-38`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.h#L36-L38) ```cpp // The single place that turns every input source into typed object storage configuration. // After it, factories, holders, file systems and readers never read raw properties, Thrift or // Protobuf parameters again. ``` **What is wrong.** The header says "single place" and "every input source", and the body lists AI embed, Kinesis, benchmarks and spill as resolver users. Three of them build typed configuration by hand, and no production spill file changed. **Why it happens.** [`embed.h:436-458`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/exprs/function/ai/embed.h#L436-L458) maps JSON fields itself, [`data_consumer.cpp:686-713`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/load/routine_load/data_consumer.cpp#L686-L713) fills `S3AuthConfig` itself, and [`file_cache_microbench.cpp:1489-1516`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/tools/file_cache_microbench.cpp#L1489-L1516) builds `ObjClientConfig` by hand. **When it bites.** The next person who adds a source trusts the header and looks in the wrong place. **Suggested fix.** Say "the flat sources (FE properties, TS3StorageParam, ObjectStoreInfoPB)" in the header, and move those three callers out of the resolver sentence in the body. ### F-10 · Nothing pins the RFC 3986 flag, which now rests on one line after `InitAPI` - **Severity**: Minor - **Category**: test-coverage (AWS SDK URI encoding) - **Regression**: no - Regression evidence: the merge base had no test either, and this head reaches the same state. - **Where**: [`be/src/util/s3_util.cpp:77-81`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/util/s3_util.cpp#L77-L81) ```cpp Aws::InitAPI(_aws_options); // Object keys must be encoded strictly by RFC 3986 in request URIs. The setting is global // to the SDK, so it is applied once here; every object storage client is built after this // factory (ObjClientFactory initializes it first). Aws::Http::SetCompliantRfc3986Encoding(true); ``` **What is wrong.** The order is correct, but no test notices if the line is lost or moved above `InitAPI`. **Why it happens.** `InitAPI` applies `httpOptions.compliantRfc3986Encoding`, which defaults to false (`thirdparty/installed/include/aws/core/Aws.h:84`), and `Aws.cpp.o` in the pinned libaws-cpp-sdk-core.a references `SetCompliantRfc3986Encoding`. Before this PR, every reader and writer re-applied the flag, so one misplaced line could not lose it for the whole process. **When it bites.** A later change to this constructor silently returns object keys with reserved characters to legacy encoding. **Suggested fix.** Use the SDK option, which removes the ordering hazard, and assert it: set `_aws_options.httpOptions.compliantRfc3986Encoding = true` before `InitAPI`, then add `S3ClientFactory::instance(); EXPECT_TRUE(Aws::Http::s_compliantRfc3986Encoding);` to `S3ClientFactoryTest`. ### F-11 · Nothing pins the BE rate limiter on Azure clients - **Severity**: Minor - **Category**: test-coverage (Azure rate limiting) - **Regression**: no - Regression evidence: behaviour is unchanged. At the merge base one statement wrapped both backends, so the S3 tests covered Azure too. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:245-248`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_client_factory_test.cpp#L245-L248) ```cpp ASSERT_TRUE(first.has_value()) << first.error(); auto cached = factory.get_or_create(resolve(param)); ASSERT_TRUE(cached.has_value()) << cached.error(); EXPECT_EQ(cached.value(), first.value()); ``` **What is wrong.** Removing the wrapper from [`azure_client_provider.cpp:99`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/azure_client_provider.cpp#L99) passes every suite. **Why it happens.** The call is now duplicated in two providers, and the rate limit tests in `be/test/io/s3_client_factory_test.cpp` drive S3 configs only. **When it bites.** Azure traffic silently stops honouring the BE limits. Requests still succeed, and no pipeline in the repo reaches Azure. **Suggested fix.** Under a cloud-mode guard, assert `dynamic_pointer_cast<RateLimitedObjStorageClient>` for non-cloud external, cloud internal and cloud external. Use distinct endpoints, since the provider cache is process-wide. ### F-12 · The Azure arm of `merge_obj_client_config` has no test - **Severity**: Minor - **Category**: test-coverage (Azure reset merge) - **Regression**: no - Regression evidence: the same fields merge as at the merge base, where one flat merge served every provider and the S3 tests pinned it. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:339-343`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_client_factory_test.cpp#L339-L343) ```cpp // Another backend is another storage target, which needs a new binding. auto as_azure = merge_obj_client_config(current, resolve(make_azure_param("merge-target.example.com"))); ASSERT_FALSE(as_azure.has_value()); EXPECT_TRUE(as_azure.error().is<ErrorCode::INVALID_ARGUMENT>()) << as_azure.error(); ``` **What is wrong.** The only Azure input in `be/test` returns on the backend mismatch before the arm at [`obj_storage_config.cpp:156-160`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L156-L160) runs. **Why it happens.** Every `reset` and merge test uses an S3 config. **When it bites.** Deleting `merged.auth = update.auth` passes every suite. A key-only update then yields `merged == current` and `reset` returns OK at [`obj_client_holder.cpp:49-51`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.cpp#L49-L51), so an Azure key rotation silently does nothing. **Suggested fix.** Add an Azure pair to `ResetMergeKeepsTheStorageTarget`: another endpoint, account, container and key; assert the endpoint stayed and the other three followed. Use a key literal that does not trip gitleaks, see F-03. ### F-13 · GCP credentials are tested on one of the three sources the PR claims - **Severity**: Minor - **Category**: test-coverage (resolver credential conversion) - **Regression**: no - Regression evidence: the merge base had no test for these conversions either. The PR improves coverage and stops short of its claim. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:419-424`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_client_factory_test.cpp#L419-L424) ```cpp auto from_vault = ObjStorageConfigResolver::from_object_store_info(vault); EXPECT_EQ(from_vault.bucket, "vault-bucket"); EXPECT_EQ(from_vault.prefix, "vault-prefix"); // A storage vault is a Doris internal bucket. EXPECT_EQ(from_vault.client.usage, ClientUsage::INTERNAL_STORAGE); EXPECT_EQ(from_vault.client.provider(), ObjStorageProvider::COS); ``` **What is wrong.** The body says the resolver converts the credential "for all three sources"; only `from_properties` is tested. **Why it happens.** Nothing in `be/test` sets a credential on a `TS3StorageParam` or `ObjectStoreInfoPB`. **When it bites.** Deleting [`obj_storage_config_resolver.cpp:300`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L300) or [`obj_storage_config_resolver.cpp:360-362`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L360-L362) passes every suite, and every read on that vault or resource then fails at runtime. The GCP IAM suite is skipped unless `objectStorageIamProvider` is set. **Suggested fix.** Add a GCP vault case and a `TS3StorageParam` case asserting `GcpCredentialConfig` reaches `S3AuthConfig::credential`, and one `Provider_AZURE` vault case. Assert `role_arn`, `external_id` and `cred_provider_type` on the existing vault. ### F-14 · Stale includes and an unused export remain after the move - **Severity**: Nit - **Category**: maintainability (include hygiene) - **Regression**: no - Regression evidence: no behaviour change. - **Where**: [`be/src/exprs/function/ai/embed.h:33`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/exprs/function/ai/embed.h#L33) ```cpp #include "util/s3_util.h" ``` **What is wrong.** `embed.h` no longer uses this header, [`s3_util.cpp:49-66`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/util/s3_util.cpp#L49-L66) keeps includes for the rate limiter, the GCP token provider, `scanner_scheduler.h` and `exec_env.h` that nothing there needs, and `to_obj_credential_provider` is exported at [`obj_storage_config.h:149`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L149) although only its own file calls it. **Why it happens.** Leftovers from moving the code out. **When it bites.** Slower rebuilds and a misleading dependency graph. **Suggested fix.** Drop them, and move the function into the anonymous namespace. ### F-15 · The new 429 comment states something false about S3 - **Severity**: Nit - **Category**: wording (comment) - **Regression**: no - Regression evidence: the value equals the old enum, so behaviour is unchanged. - **Where**: [`be/src/io/fs/s3_file_reader.cpp:47-48`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/fs/s3_file_reader.cpp#L47-L48) ```cpp // Throttling by the storage service, which every backend reports as HTTP 429. constexpr int OBJ_STORAGE_HTTP_TOO_MANY_REQUESTS = 429; ``` **What is wrong.** AWS S3 reports throttling as 503 SlowDown ([`obj_retry_strategy.cpp:49-50`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/common/cpp/obj_retry_strategy.cpp#L49-L50)), which does not take this branch. **Why it happens.** The enum name was replaced by a sentence that generalises it. **When it bites.** A reader concludes that the retry at [`s3_file_reader.cpp:170`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/fs/s3_file_reader.cpp#L170) covers S3 throttling. **Suggested fix.** "HTTP 429 from the storage service. S3's 503 SlowDown is retried inside the SDK client instead." ### F-16 · The "never a mix" example uses two calls that take two snapshots - **Severity**: Nit - **Category**: wording (concurrency comment) - **Regression**: no - Regression evidence: no behaviour change. - **Where**: [`be/src/io/obj_storage/obj_client_holder.h:32-35`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.h#L32-L35) ```cpp // One immutable generation of an ObjClientHolder. The config and the client built from it // are published together, so a caller never pairs a new config with an old client. For // example, a reset from ak1 to ak2 that races with full_s3_path() and get() lets each // caller see either (ak1 config, ak1 client) or (ak2 config, ak2 client), never a mix. ``` **What is wrong.** `get()` and `full_s3_path()` lock separately, so a caller using both can see a mix. Only one `state()` snapshot gives the guarantee. **Why it happens.** The example names the convenience accessors rather than `state()`. **When it bites.** It is harmless today, because `reset` never changes the endpoint, which is all `full_s3_path` reads. `be/AGENTS.md` asks that examples match the code. **Suggested fix.** Base the example on `state()`, as [`s3_file_system.cpp:383`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/fs/s3_file_system.cpp#L383) does. ### F-17 · The PR body still names `merge_update` - **Severity**: Nit - **Category**: wording (stale PR body) - **Regression**: no - Regression evidence: no behaviour change. - **Where**: [`be/src/io/obj_storage/obj_storage_config.h:156-162`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L156-L162) ```cpp Result<ObjClientConfig> merge_obj_client_config(const ObjClientConfig& current, const ObjClientConfig& update); ``` **What is wrong.** `rg merge_update be` is empty at this head; commit af4964b9ceb moved it into the config layer under a new name. **Why it happens.** The body was not updated after the last commit. **When it bites.** The body becomes the squash commit message. **Suggested fix.** Use `merge_obj_client_config`, move the sentence out of the resolver bullet, and mention `validate_obj_client_config`. ### F-18 · The microbench reader sets a key nothing reads - **Severity**: Nit - **Category**: functional-bug (microbench tool config) - **Regression**: no - Regression evidence: same key at the merge base. The PR rewrites these lines. - **Where**: [`be/src/io/tools/file_cache_microbench.cpp:1806-1807`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/tools/file_cache_microbench.cpp#L1806-L1807) ```cpp props["AWS_CONNECT_TIMEOUT_MS"] = std::to_string(client_config.connection.connect_timeout_ms); ``` **What is wrong.** The resolver key is `AWS_CONNECTION_TIMEOUT_MS` ([`obj_storage_config_resolver.cpp:78`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L78)). **Why it happens.** Old typo, kept when the lines were rewritten. **When it bites.** The read client runs without the 3000 ms connect timeout, so reads and writes use two cached clients with different timeouts. **Suggested fix.** Use `AWS_CONNECTION_TIMEOUT_MS`. ### F-19 · Neither `empty endpoint` branch has a test - **Severity**: Nit - **Category**: test-coverage (endpoint validation) - **Regression**: no - Regression evidence: the merge base had no such test either. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:360-365`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_client_factory_test.cpp#L360-L365) ```cpp // Azure has no region; its SharedKey account and key come together, like ak/sk. auto azure_no_key = make_azure_param("validate-azure.example.com"); azure_no_key.__set_sk(""); auto result = factory.get_or_create(resolve(azure_no_key)); ASSERT_FALSE(result.has_value()); EXPECT_TRUE(result.error().is<ErrorCode::INVALID_ARGUMENT>()) << result.error(); ``` **What is wrong.** Deleting [`obj_storage_config.cpp:95-97`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L95-L97) or [`obj_storage_config.cpp:140-142`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.cpp#L140-L142) passes every suite. The Azure one is new code. **Why it happens.** `FactoryValidatesBeforeBuilding` covers region and keys only. **When it bites.** The Azure mutant attempts a client for `https:///container`. **Suggested fix.** Add one S3 and one Azure case with an empty endpoint to `FactoryValidatesBeforeBuilding`. ### F-20 · The `#` half of the rewritten URI strip is untested - **Severity**: Nit - **Category**: test-coverage (URI parsing) - **Regression**: no - Regression evidence: `find_first_of` is behaviourally equal to the old double split, checked on 559870 inputs. - **Where**: [`be/test/io/obj_storage/obj_storage_uri_test.cpp:96-102`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/test/io/obj_storage/obj_storage_uri_test.cpp#L96-L102) ```cpp TEST_F(ObjStorageUriS3RulesTest, QueryAndFragment) { std::string p1 = "s3://bucket/path/to/file?query=foo#bar"; ``` **What is wrong.** Changing `"?#"` to `"?"` at [`obj_storage_uri.cpp:64`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_uri.cpp#L64) passes every suite. **Why it happens.** Every test input has `?` before `#`. **When it bites.** Only if that line is edited again. **Suggested fix.** Add `s3://bucket/path#frag` and `s3://bucket/a#b?c`, both expecting the key cut at `#`. <details> <summary>Critical checkpoints</summary> | Checkpoint | Conclusion | |---|---| | Goal achieved, proven by a test | Yes for S3 and Azure resolution and identity: `ObjClientFactoryTest` 11/11, `ObjStorageUriS3RulesTest` 9/9, `S3ClientFactoryTest` 38/38 pass on TeamCity build 1082093. Azure reset and rate limiting are not proven: F-11, F-12 | | Change is minimal, clear, focused | Large but mostly a move. Leftovers in F-14 | | Concurrency: threads, shared state, locks, lock order | State is an immutable `shared_ptr` under `_mtx`; readers copy under a shared lock; publish compares the generation under the exclusive lock ([`obj_client_holder.cpp:60-83`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.cpp#L60-L83)); builds and destruction run outside locks; no nested locks. Arrival order against freshness is F-07 | | Lifecycle / static init order | [`obj_client_factory.cpp:26-31`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_factory.cpp#L26-L31) finishes `S3ClientFactory` first, so cached clients die before `ShutdownAPI`, an improvement on the merge base. No new cross-TU initializer | | New config: dynamic? observed without restart? | n/a, none added. An existing option changed scope: F-05 | | Incompatible change: rolling-upgrade path | No wire or storage format change, gensrc untouched. Capacity changes on upgrade: F-05 | | Parallel code paths updated | Every merge-base use of a removed API is migrated. Master has since added two users: F-02 | | Special conditionals explained | Commented. Accuracy issues are F-07 and F-08. [`cloud_storage_engine.cpp:153`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/cloud/cloud_storage_engine.cpp#L153) discriminates the variant and is not defensive | | Test coverage (e2e, negative, unit) | No migrated case or assertion was weakened: 38 of 38 and 8 of 8 kept. Gaps: F-10 to F-13, F-19, F-20. No new regression suite is needed for this refactor, but no pipeline reaches Azure, so unit tests are the only gate there | | Test results added/modified and correct | n/a, no `.out` or groovy file changed | | Observability (logs, VLOG, metrics) | Logs preserved, secrets masked. F-06; F-04's WARNING has no resource id; pool wait is invisible, see F-05 | | Transaction / persistence / EditLog replay | n/a, none touched | | Data write atomicity, crash safety | Writer logic untouched. F-04 misplaces cold data; F-01 is a BE crash | | New FE↔BE variables on every send path | n/a, none added. F-01 is a disagreement on an existing field | | Performance and anti-patterns | Hot paths unchanged, `get()` is the same shared lock plus a pointer copy. F-05 | | Anything else | F-03 blocks merge. No other required gate is red; the TeamCity results are stale, see F-02 | </details> <details> <summary>Considered and dismissed</summary> | # | Concern | Where | Why it is not an issue | |---|---|---|---| | D-01 | A later `InitAPI` resets the RFC 3986 flag | [`s3_util.cpp:81`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/util/s3_util.cpp#L81) | It is the only `InitAPI` in `be/src`; libarrow, libparquet, libpaimon_c and libarrow_dataset have no undefined Aws symbols; it runs at startup ([`exec_env_init.cpp:479`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/runtime/exec_env_init.cpp#L479)). On earlier is the state every BE already reached after its first reader | | D-02 | `DORIS_CHECK(s3 != nullptr)` in `generate_presigned_url` | [`s3_file_system.cpp:388`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/fs/s3_file_system.cpp#L388) | The only filesystem caller uploads through the same client first ([`runtime_state.cpp:592-601`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/runtime/runtime_state.cpp#L592-L601)), so an Azure SDK write would have to succeed against an aliyuncs internal endpoint | | D-03 | `USE_AZURE` code never compiled in CI | [`azure_client_provider.cpp:105`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/azure_client_provider.cpp#L105) | `build.sh` and `run-be-ut.sh` default `BUILD_AZURE` to ON; COMPILE and BE UT passed at this head | | D-04 | Non-default targets do not compile | [`s3_benchmark.hpp:42-44`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/fs/benchmark/s3_benchmark.hpp#L42-L44) | `be/src/io/fs/benchmark/s3_benchmark.hpp` and `be/src/io/tools/file_cache_microbench.cpp` were read as a compiler would, every type, capture and designated initializer. No defect. They were not compiled | | D-05 | Unity batching collisions from the new files | [`CMakeLists.txt:66`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/CMakeLists.txt#L66) | All 38 new internal names are unique across `be/src/io` and `be/src/util`, also on the merged tree | | D-06 | Sharing a client lets two buckets interfere semantically | [`obj_storage_config.h:80-90`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L80-L90) | No per-client state is bucket-scoped: no region cache, no bucket-scoped signer or credential, no retry quota. Capacity is F-05 | | D-07 | Pool exhaustion can deadlock | [`s3_client_provider.cpp:72-76`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/s3_client_provider.cpp#L72-L76) | No cycle: a handle holder blocks only on the network, signing and token fetches happen before acquisition, and retry backoff releases the handle | | D-08 | `Aborted` on the cold-storage path | [`agent_server.cpp:145-146`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/agent/agent_server.cpp#L145-L146) | One `PUSH_STORAGE_POLICY` worker, registered only in non-cloud mode, and `init()` runs before a filesystem is published | | D-09 | Cached clients outlive `Aws::ShutdownAPI` | [`obj_client_factory.cpp:26-31`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_factory.cpp#L26-L31) | True for clients held only by the cache. Production exits with `_exit(0)`. Better than the merge base | | D-10 | Behavioural conflict with master beyond F-02 | master tip | Master made no commit on any object-storage path, added no `TS3StorageParam` or `ObjectStoreInfoPB` field, no provider and no credential source. The compile surface is exactly F-02 | | D-11 | The `merged == current` short circuit swallows a needed rebuild | [`obj_client_holder.cpp:49-51`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_client_holder.cpp#L49-L51) | In a 1890-cell reset model, the 94 new no-ops are S3 bucket only and Azure token, role and addressing, none of which a client reads. The base rebuild was never a repair path | | D-12 | Azure loses behaviour by dropping S3-only fields | [`obj_storage_config_resolver.cpp:125-130`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L125-L130) | `git show b8dc326111ff:be/src/util/s3_util.cpp` lines 314-364 never read them. The role cell is F-08 | | D-13 | Duplicate key spellings on FILE_S3 behave differently | [`obj_storage_config_resolver.cpp:195`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L195) | Same `StringCaseMap` construction as the merge base, and this path returns a Status | | D-14 | `sse_enabled` dropped | [`obj_storage_config.h:138-141`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/obj_storage_config.h#L138-L141) | The merge base only assigned and logged it | | D-15 | Azure account now unmasked in logs | [`azure_client_provider.cpp:135-136`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/azure_client_provider.cpp#L135-L136) | The account is already in the endpoint host. The key is never printed | | D-16 | `check_coverage` is red | CI | Infrastructure: TeamCity build 1082078 failed fetching the compile tarball from OSS (404). Not a code signal | | D-17 | Malformed Azure endpoint port throws on a thread with no catch | [`azure_client_provider.cpp:130`](https://github.com/apache/doris/blob/af4964b9ceb3e331be2c6c0ab62d7a9a162673f0/be/src/io/obj_storage/azure_client_provider.cpp#L130) | Real, but identical at the merge base: `Azure::Core::Url` throws from `AzureAuthFactory::create`. Not this PR. Worth a separate issue | </details> <details> <summary>Coverage and limits</summary> - Files reviewed in depth: all 26 core-layer and filesystem files and all 13 production callers were read in full, on both sides. The merge-base `be/src/util/s3_util.cpp`, `be/src/util/s3_uri.cpp` and `be/src/io/fs/s3_file_system.cpp` were reconstructed and compared by hand and with exhaustive models. - Files skimmed and why: seven large test files were read as PR hunks plus every object-storage touchpoint, because the rest is unchanged: `be/test/ai/embed_test.cpp`, `be/test/cloud/cloud_compaction_test.cpp`, `be/test/io/client/s3_file_system_test.cpp`, `be/test/io/fs/s3_file_writer_test.cpp`, `be/test/storage/cloud_file_cache_write_index_only_test.cpp`, `be/test/storage/index/inverted/snii/bench/snii_vs_v3_benchmark_test.cpp`, `be/test/vec/spill/spill_file_s3_test.cpp`. All their hunks are mechanical migrations. - Not verified locally: no build and no test was run. F-01's statements were not executed on a cluster. Model figures in F-05 are modelled, not measured. The build hygiene gate was run on a scratch export of the trial merge, which is a text scan. - Evidence taken from outside the repository: aws-sdk-cpp 1.11.221 sources at the pinned tag and `libaws-cpp-sdk-core.a`; Azure SDK headers under `thirdparty/installed`; the gitleaks report artifact of run 40338187924; TeamCity results for builds 1082093, 1082174 and 1082078; master tip `8e10b235cf03` and a trial merge tree. - Inherited from earlier runs: `719de6a` (2026-10-26). Twelve of its 13 dismissals were re-confirmed and carried forward. Its D-07 was reopened and is F-01; its D-08 is refined by F-08. Its Blocker is fixed by commit 7d30bee1d9d and its Minor by item 4 of the PR body. - Convergence: converged. The verdict has been stable since round 1 and the blocking set since round 2; round 3 added nothing. Rounds 1 and 2 did not converge on their own because each added Blocker or Major findings. </details> <!-- doris-repo-review:report:end --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
