morningman commented on PR #69605: URL: https://github.com/apache/doris/pull/69605#issuecomment-6618322006
<!-- doris-repo-review:report:begin --> Review at head `f58598d426a9`. 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 | `f58598d426a9d97a110dba1f1e85f6279f0607b9` on `refactor/be-obj-client-config` of `xylaaaaa/doris` | | Diff range | `7acfd9bfc27f01f0b953e8ac0cdc3482a46e3c59...f58598d426a9d97a110dba1f1e85f6279f0607b9` (three-dot, merge base `7acfd9bfc27f01f0b953e8ac0cdc3482a46e3c59`) | | Scope | 69 files, +3541/-1724 lines, 14 commits (the head is a merge of master) | | Verdict | **REQUEST_CHANGES** | | Rounds | 2 of max 3; converged (verdict stable since round 1; round 2 added no finding and refined F-01 and F-02) | | Builds on | `719de6a` (2026-10-26) and `af4964b` (2026-10-31) | ## Summary The PR replaces the flat `S3Conf` / `S3ClientConf` with a typed configuration that holds exactly one backend, and builds S3 and Azure clients through per-backend providers. This is the third review. Since `af4964b` the author has fixed all 20 findings of the previous review. The BE no longer aborts on an Azure resource that carries a native credential. The gitleaks gate is green, and so is BE UT with master merged. The connection-limit scope is declared. Each test gap the previous review named now has a test that fails on the named mutation. Every TeamCity gate passed at this head. Two Majors remain, and both are in the new code that applies a backend change. The fix for the old F-04 makes `S3FileSystem::refresh` build a second file system. Rowsets that already resolved the first one keep it until BE restart, so a revert or a later key rotation never reaches them (F-01). The cloud vault refresh did not get that fix and still only resets. A vault id that is handed out again for the other provider therefore keeps the dropped vault's target, and loads write there (F-02). The rest is a false rationale in two comments (F-03) and three test gaps (F-04 to F-06). ## Findings at a glance | ID | Severity | Regression | Finding | |---|---|---|---| | F-01 | Major | yes | A provider change across S3 and Azure leaves already-bound rowsets on the old file system, so a revert or a key rotation no longer reaches them | | F-02 | Major | yes | A vault id that is handed out again for another provider keeps the dropped vault's file system, and loads write into the old bucket | | F-03 | Minor | no | Two new comments give a false reason why Recycler does not check the Azure key pair | | F-04 | Minor | no | No test that runs in CI pins which value becomes the Azure SharedKey account and which the key | | F-05 | Nit | no | `ObjClientFactoryTest` counts builds through a process-wide cache and cannot be repeated in one process | | F-06 | Nit | no | The refresh test does not pin a vendor change inside the S3 family | ## Status of the previous review's findings The previous review is [this comment](https://github.com/apache/doris/pull/69605#issuecomment-6546301394). | Prior (af4964b) | Severity | At this head | |---|---|---| | F-01 Azure plus a native credential aborts the BE | Blocker | Fixed. The resolvers return `Result`, and the Azure mapping returns InvalidArgument for all three sources ([`obj_storage_config_resolver.cpp:111-120`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L111-L120)), tested at [`obj_client_factory_test.cpp:202-236`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/obj_storage/obj_client_factory_test.cpp#L202-L236) | | F-02 BE UT breaks once merged into master | Blocker | Fixed. The CSV, JSON and S3 reader tests were migrated and BE UT 1090345 is green; master tip `7866f5a36c0` adds no user of a removed symbol | | F-03 required `Check for secrets` red | Blocker | Fixed. Commit 1 was rewritten and the check is green | | F-04 a running BE ignores a provider-family ALTER | Major | Fixed for the registry by `S3FileSystem::refresh`. That fix causes F-01 below, and the vault path is F-02 | | F-05 connection-limit scope undeclared | Major | Fixed. Item 1, the release note and [`obj_storage_config.h:49-51`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_storage_config.h#L49-L51) | | F-06 to F-09 | Minor | Fixed: the vault sync logs the failing status, and the reset comment, item 2, the role_arn wording and the resolver header now match the code | | F-10 to F-13 | Minor | Fixed. Each mutation the previous review named now fails a test that ran in BE UT 1090345 | | F-14 to F-20 | Nit | Fixed | ## Findings ### F-01 · A provider change across S3 and Azure leaves already-bound rowsets on the old file system, so a revert or a key rotation no longer reaches them - **Severity**: Major - **Category**: functional-bug (lifecycle) - **Regression**: yes - Regression evidence: `git show 7acfd9bfc27:be/src/io/fs/s3_file_system.cpp` lines 83-123 apply every update, provider included, to the one holder of a resource, and the base `update_s3_resource` keeps `existed_fs`. Item 3 promises "the same result as after a restart" and says that a policy-referenced resource "still fails on access, as before". It does not say that rowsets which already resolved the resource stay on the old backend. - **Where**: [`be/src/io/fs/s3_file_system.cpp:78-86`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L78-L86) ```cpp Result<std::shared_ptr<S3FileSystem>> S3FileSystem::refresh(std::shared_ptr<S3FileSystem> fs, ObjStorageConfig config) { const auto& holder = fs->client_holder(); if (holder->state()->config.backend.index() != config.client.backend.index()) { return create(std::move(config), fs->id()); } RETURN_IF_ERROR_RESULT(holder->reset(config.client)); return fs; } ``` **What is wrong.** For another backend, `refresh` builds a second file system with the same id, and `update_s3_resource` registers it ([`task_worker_pool.cpp:1809-1828`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/agent/task_worker_pool.cpp#L1809-L1828)). Nothing re-points the objects that already hold the first one. A remote rowset binds its storage resource once, lazily on first use or eagerly at cooldown, and never resolves it again ([`rowset_meta.cpp:199-201`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/storage/rowset/rowset_meta.cpp#L199-L201), [`rowset_meta.cpp:220-223`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/storage/rowset/rowset_meta.cpp#L220-L223)). Its segments open through that file system ([`beta_rowset.cpp:278`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/storage/rowset/beta_rowset.cpp#L278)) and read through that file system's holder ([`s3_file_system.cpp:136`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L136), [`s3_file_reader.cpp:134`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_reader.cpp#L134)). Later pushes refresh only the registered object ([`task_worker_pool.cpp:1872-1878`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/agent/task_worker_pool.cpp#L1872-L1878)). The holder exists so that cached readers follow every update ([`obj_client_holder.h:46-48`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_client_holder.h#L46-L48)), and at the merge base each resource had exactly one. **Why it happens.** Commit b343fc6cd2e assumes that only in-flight requests hold the old file system ("requests that hold the old one finish on it"). The new test asserts the split as intended ([`obj_client_factory_test.cpp:529-530`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/obj_storage/obj_client_factory_test.cpp#L529-L530)). On the FE side, a resource referenced by a storage policy keeps its endpoint, region, root path and bucket frozen, but not its provider ([`S3Resource.java:250-263`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java#L250-L263)). The ping is skipped when `s3_validity_check` is false ([`S3Resource.java:284-297`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java#L284-L297)). When the ping runs, it goes through the first filesystem plugin that accepts the properties. Plugins load in directory-name order, so Azure comes first ([`DirectoryPluginRuntimeManager.java:213-216`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java#L213-L216), [`FileSystemPluginManager.java:203-209`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemPluginManager.java#L203-L209)). The Azure plugin accepts a map that says `provider=azure` or that names an Azure blob endpoint ([`AzureFileSystemProvider.java:63-80`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/fe/fe-filesystem/fe-filesystem-azure/src/main/java/org/apache/doris/filesystem/azure/AzureFileSystemProvider.java#L63-L80)). For an S3 resource, the switch to azure therefore fails the ping and needs `s3_validity_check=false`. For an Azure resource, both steps of a round trip through `provider=S3` ping the unchanged Azure target and pass with every setting at its default. **When it bites.** Either of two cases triggers it on a non-cloud cluster. One is an Azure resource with default settings that is switched to S3 and back. The other is an S3 resource with `s3_validity_check=false`, a setting the regression suites use 47 times across 40 files, that is switched to azure and back. The table shows the S3 case, for a resource `r` that is referenced by a storage policy and has cooled rowsets. | Step | Merge base | This head | |---|---|---| | 1. `ALTER RESOURCE r PROPERTIES ("provider"="azure")` by mistake | The one holder becomes an Azure client on the frozen S3 endpoint, and every read of cooled data fails | A second, Azure file system is registered. Rowsets bound before the ALTER keep reading S3. Rowsets first opened now bind to the Azure one and fail | | 2. `ALTER RESOURCE r PROPERTIES ("provider"="S3")` to revert | The holder goes back to S3 and every reader recovers | A third, S3 file system is registered. The rowsets bound in step 1 keep failing until BE restart | | 3. Rotate `s3.access_key` / `s3.secret_key`, then revoke the old key | Every reader uses the new key | Only the third file system gets it. Rowsets bound before step 1 fail with 403 until BE restart | FE shows the corrected resource and the BE logs "successfully update s3 resource", so nothing points at the stale binding. Rowsets can also bind during the window of step 1 without any user query. Cold data compaction on the cooldown replica reads its inputs every 1800 seconds, and rowsets loaded at a restart or followed from another replica start unbound. No data is lost: the objects stay where they were. This was traced in the code and was not run on a cluster. **Severity rationale.** It is a regression in `functional-bug`, so it is at least Major. It is not a Blocker. No data is lost and no result is wrong, because reads fail with an error. A BE restart heals it, and the trigger needs ADMIN. In one cell this head is better than the merge base. During the mistaken window, rowsets bound before step 1 keep working, where the base failed all of them. The flip side is that the broken resource stays hidden until the next restart. **Suggested fix.** Keep one holder for the readers as long as the storage target stays. The backend may change while the bucket and the normalized prefix stay the same; under a storage policy that is always the case, because FE freezes both. Then rebind the existing holder in place to a client built from `config.client` as it is, without merging. Validate it through `ObjClientFactory`, publish it with the existing generation check, and return `fs`. Build a new file system only when the bucket or the prefix moves, which is the case the old F-04 needs. A sketch, with illustrative helper names: ```diff const auto& holder = fs->client_holder(); - if (holder->state()->config.backend.index() != config.client.backend.index()) { - return create(std::move(config), fs->id()); - } - RETURN_IF_ERROR_RESULT(holder->reset(config.client)); + if (holder->state()->config.backend.index() == config.client.backend.index()) { + RETURN_IF_ERROR_RESULT(holder->reset(config.client)); + return fs; + } + // Rowsets that already resolved this resource keep `fs` and its holder until BE restart, + // so keep the holder while the target stays (FE freezes bucket and root path under a + // storage policy). Only a moved target needs another file system. + if (config.bucket != fs->bucket() || normalized_prefix(config.prefix) != fs->prefix()) { + return create(std::move(config), fs->id()); + } + RETURN_IF_ERROR_RESULT(holder->rebind(config.client)); return fs; ``` Add a test that keeps `auto reader = fs->client_holder();` across S3, then Azure, then S3 again, with the same bucket and root path. Rotate the key through the registered file system and assert that `reader` has the rotated key. If the split is meant to stay, say so in item 3 and in [`s3_file_system.h:50-53`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.h#L50-L53): already-bound rowsets stay on the old backend until BE restart. As an FE follow-up, reject a provider change between Azure and the S3-compatible providers under a storage policy. ### F-02 · A vault id that is handed out again for another provider keeps the dropped vault's file system, and loads write into the old bucket - **Severity**: Major - **Category**: functional-loss (cloud storage vault refresh) - **Regression**: yes - Regression evidence: `git show 7acfd9bfc27:be/src/cloud/cloud_storage_engine.cpp` calls the same reset. The base reset merges `provider` into the kept target (`git show 7acfd9bfc27:be/src/io/fs/s3_file_system.cpp` lines 83-123). The client therefore switched to the new provider against the old endpoint, and every request failed: `git show 7acfd9bfc27:be/src/util/s3_util.cpp` lines 315-319 build the Azure URI from the kept endpoint. Item 3 says "Storage vaults cannot change their provider", which holds for ALTER but not for a reused id. - **Where**: [`be/src/cloud/cloud_storage_engine.cpp:204`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L204) ```cpp const auto& s3_conf = resolved.value(); DCHECK_EQ(fs->type(), io::FileSystemType::S3) << id; auto s3_fs = std::static_pointer_cast<io::S3FileSystem>(fs); auto client_holder = s3_fs->client_holder(); auto st = client_holder->reset(s3_conf.client); if (!st.ok()) { LOG(WARNING) << "failed to update s3 fs, resource_id=" << id << ": " << st; } return st; ``` **What is wrong.** The fix for the old F-04 went into `update_s3_resource` only. The vault refresh still calls `reset`, which now refuses another backend ([`obj_storage_config.cpp:207-212`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_storage_config.cpp#L207-L212)). The BE keeps the file system it registered for the id, and no other path replaces it. **Why it happens.** ALTER cannot change the provider of a vault ([`meta_service_resource.cpp:1089-1096`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L1089-L1096)), but a drop can free its id. The MS HTTP API `drop_hdfs_vault` removes any vault by name ([`http_helper.cpp:524`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/common/http_helper.cpp#L524), [`meta_service_resource.cpp:1699-1704`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L1699-L1704), [`meta_service_resource.cpp:892-915`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L892-L915)), and it is the only drop the MS accepts ([`meta_service_resource.cpp:1744-1751`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/ cloud/src/meta-service/meta_service_resource.cpp#L1744-L1751)). A new vault gets the largest existing id plus one ([`meta_service_resource.cpp:673-697`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L673-L697)), which is the dropped id when that vault held the largest. The BE never unregisters a vault: `delete_storage_resource` ([`storage_policy.cpp:127`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/storage/storage_policy.cpp#L127)) has no caller. Sync therefore finds the old file system under the id ([`cloud_storage_engine.cpp:496-502`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L496-L502)), and loads resolve it ([`cloud_rowset_builder.cpp:115`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_rowset_builder.cpp#L115)). **When it bites.** A cloud cluster in vault mode: | Step | Merge base | This head | |---|---|---| | 1. `CREATE STORAGE VAULT s3v` (S3, endpoint E1, bucket b1) gets id N | BEs register an S3 file system under N | Same | | 2. An operator drops `s3v` with `drop_hdfs_vault` and creates `azv` (Azure, endpoint E2, container c2), which gets N again | Sync resets the holder to an Azure client on E1, and every load into `azv` fails | Sync logs a WARNING and keeps the S3 file system. Loads into `azv` succeed, into b1 with the old keys | | 3. BE restart | `azv` is built on E2/c2 and nothing is misplaced | `azv` is built on E2/c2. The rowsets of step 2 are not there, and queries on them fail | One natural way to get here is to fix a vault created with the wrong provider by dropping it and creating it again. Only the cross-family cell changed: a same-family reuse misbinds the same way at the merge base and at this head. This was traced in the code and was not run on a cluster. **Severity rationale.** It is rated at the floor, for the same reason as the old F-04: the loud failure at the merge base protected the data, and at this head the data lands in the old bucket. It is not a Blocker for two reasons. The objects exist and can be copied by hand. And the trigger needs an operator to drop an object vault through an API named for HDFS vaults, a gap in the meta service that predates this PR. **Suggested fix.** The root fix belongs in the meta service: never hand out a dropped vault id again, and make `remove_hdfs_storage_vault` refuse an object vault. A BE change alone is not enough. Registering a new file system from `RefreshFSVaultVisitor`, through `S3FileSystem::refresh` and `put_storage_resource(id, {fs, path_format}, 0)`, would fix the registry. But `PackedFileManager` keeps its own file system per vault id for the life of the process ([`packed_file_manager.cpp:292-296`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/packed_file_manager.cpp#L292-L296)), so the packed small files of new loads would still go to the old bucket. A vault id reused from an HDFS vault also reaches this visitor with an `HdfsFileSystem`, so a BE fix must check `fs->type()` before the cast at [`cloud_storage_engine.cpp:202`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L202); that cast is the same at the merge base. If the BE side changes in this PR, either evict that cache entry too, or make loads for an id whose backend changed fail instead of keeping the old target. Then qualify item 3. ### F-03 · Two new comments give a false reason why Recycler does not check the Azure key pair - **Severity**: Minor - **Category**: wording (cross-component invariant) - **Regression**: no - Regression evidence: BE and Recycler behave as at the merge base for every stored shape; only the stated reason is wrong. - **Where**: [`cloud/src/recycler/s3_accessor.cpp:399-401`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/recycler/s3_accessor.cpp#L399-L401) and [`common/cpp/obj-client/auth/azure/azure_auth.h:49-51`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/common/cpp/obj-client/auth/azure/azure_auth.h#L49-L51) ```cpp // Storage metadata carries the SharedKey pair as ak/sk, which the meta service requires // together for Azure when it stores them; the SDK credential comes from the // authentication shared with BE. ``` **What is wrong.** The meta service requires the pair only when an Azure entry is created. ALTER STORAGE VAULT checks only that both keys are sent, not that both are non-empty ([`meta_service_resource.cpp:1180-1194`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L1180-L1194)). The MS HTTP calls `alter_obj_info`, `legacy_update_ak_sk` and `update_ak_sk` do the same. So `ALTER STORAGE VAULT azv PROPERTIES ("type"="S3", "s3.access_key"="acct", "s3.secret_key"="")` stores an Azure vault with half a pair. **Why it happens.** Commit 0225126bc43 removed the check again, rightly, and gave this reason. The real reason is that `S3Conf::from_obj_store_info` loads ak and sk only as a pair ([`s3_accessor.cpp:276-292`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/recycler/s3_accessor.cpp#L276-L292)), so Recycler never sees half a pair. A failed accessor init would also fail the instance's recycler init. **When it bites.** The header is where the next authentication methods (SAS, OAuth2, Managed Identity) are added. Someone who trusts it will skip a check on the assumption that the meta service already validated the input. **Suggested fix.** In [`s3_accessor.cpp:399-401`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/recycler/s3_accessor.cpp#L399-L401) write that `S3Conf::from_obj_store_info` loads the pair complete or not at all. In [`azure_auth.h:49-51`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/common/cpp/obj-client/auth/azure/azure_auth.h#L49-L51) drop "that the meta service validated when it stored it". In the PR body, say that the meta service requires the pair when an Azure entry is created. ### F-04 · No test that runs in CI pins which value becomes the Azure SharedKey account and which the key - **Severity**: Minor - **Category**: test-coverage (Azure SharedKey wiring) - **Regression**: no - Regression evidence: `git show 7acfd9bfc27:common/cpp/obj-client/auth/azure_auth_factory.cpp` built the same credential, and its only test was the same empty case. The signature changed from named struct fields to adjacent positional `std::string_view` arguments, so a swap no longer stands out. - **Where**: [`be/test/io/fs/azure_obj_storage_client_test.cpp:85-90`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/fs/azure_obj_storage_client_test.cpp#L85-L90) ```cpp TEST(AzureAuthFactoryTest, AllowsEmptySharedKeyCredentials) { auto result = AzureAuthFactory::create("https://account.blob.core.windows.net/container";, "", AzureSharedKeyConfig {.account_key = ""}, {}); EXPECT_TRUE(result); } ``` **What is wrong.** Three swaps would pass both BE UT and Cloud UT: the account and the key in [`azure_auth_factory.cpp:30-31`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/common/cpp/obj-client/auth/azure/azure_auth_factory.cpp#L30-L31), `conf_.ak` and `conf_.sk` at [`s3_accessor.cpp:402-403`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/recycler/s3_accessor.cpp#L402-L403), or the container for the account at [`azure_client_provider.cpp:113-114`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/azure_client_provider.cpp#L113-L114). Every Azure request of BE and Recycler would then fail authentication. **Why it happens.** The only direct test passes empty values. The BE tests that build Azure clients check identity, the rate-limit wrapper or file-system fields. The suites that would authenticate are skipped in CI: in Cloud UT 1090347 `S3AccessorTest.azure` and the four `S3AccessorClientTest.Azure*` cases did not run, and neither did the three `AzureObjStorageClientTest` cases in BE UT 1090345. Commit 0225126bc43 still names "cloud UT in CI" as the test of the Recycler change. **When it bites.** The next change to this factory is the SAS / OAuth2 work the PR body announces. **Suggested fix.** Give `AzureAuthFactoryTest` a non-empty account and key. Then assert `result.shared_key_credential->AccountName`, the key (BE UT is built without access control), and `result.container_client->GetUrl()`. Add one Recycler case that creates an Azure `S3Accessor` from fake ak/sk and checks the credential's account name; it needs no network. ### F-05 · `ObjClientFactoryTest` counts builds through a process-wide cache and cannot be repeated in one process - **Severity**: Nit - **Category**: maintainability (test isolation) - **Regression**: no - Regression evidence: new test file. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:98-103`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/obj_storage/obj_client_factory_test.cpp#L98-L103) ```cpp _sync_point->set_call_back( "s3_client_factory::create", [this](auto&& args) { ++_s3_builds; if (_on_s3_build) { // Run once: a nested build must not re-enter the hook. ``` **What is wrong.** The hook runs only on a miss in the provider cache of the process-wide `ObjClientFactory` ([`s3_client_provider.cpp:42-59`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/s3_client_provider.cpp#L42-L59)), and nothing clears that cache. With `GTEST_REPEAT=2`, the second iteration fails `S3ProviderCachesByFullConfig`, `HolderRejectsResetMergedOntoStaleState` and `FactoryValidatesBeforeBuilding`. A single run is fine, because no other test uses these endpoints, and BE UT 1090345 ran all 16 cases. **Suggested fix.** Add a `BE_TEST`-only cache reset next to `set_client_creator_for_test` and call it in `SetUp`, or make the counted endpoints unique per test instance. ### F-06 · The refresh test does not pin a vendor change inside the S3 family - **Severity**: Nit - **Category**: test-coverage (refresh backend dispatch) - **Regression**: no - Regression evidence: new function and test. - **Where**: [`be/test/io/obj_storage/obj_client_factory_test.cpp:498-531`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/obj_storage/obj_client_factory_test.cpp#L498-L531) ```cpp TEST_F(ObjClientFactoryTest, RefreshResetsTheSameBackendAndRebuildsAnotherOne) { auto param = make_s3_param("refresh-s3.example.com"); ``` **What is wrong.** The test covers AWS to AWS and AWS to Azure. If [`s3_file_system.cpp:81`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L81) compared `provider()` instead of `backend.index()`, every suite would pass. Every ALTER from AWS to OSS, COS or GCP would then build a new file system instead of resetting the existing one in place, and F-01 would apply to those too. **Suggested fix.** Refresh with the same parameter under provider OSS and another endpoint. Assert that the same `fs` comes back, that the endpoint is kept, and that the vendor is OSS. <details> <summary>Critical checkpoints</summary> | Checkpoint | Conclusion | |---|---| | Goal achieved, proven by a test | Mostly. The typed configuration, the per-backend clients and the Azure model shared with Recycler work and are tested. In BE UT 1090345 at this head `ObjClientFactoryTest` passed 16 of 16, `ObjStorageUriS3RulesTest` 9 of 9 and `S3ClientFactoryTest` 39 of 39. The fix for the old F-04 achieves only the registry half: F-01, F-02 | | Change is minimal, clear, focused | Large, but mostly a move. Commits 5 to 13 are small, and each answers one earlier finding. 28854079f3e adds a Recycler check and 0225126bc43 takes it out again, which leaves only a comment (F-03) | | Concurrency: threads, shared state, locks, lock order | `update_s3_resource` and `S3FileSystem::refresh` run on the single PUSH_STORAGE_POLICY worker ([`agent_server.cpp:145-146`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/agent/agent_server.cpp#L145-L146)). The registry is under its mutex, and the holder's generation check handles overlapping vault resets (item 2). No new lock and no new lock order | | Lifecycle / static init order | A backend change now publishes a second file system for one resource id, and readers that hold the first keep it until restart: F-01. No new static and no cross-TU initializer | | New config: dynamic? observed without restart? | n/a, no option added | | Incompatible change: rolling-upgrade path | No wire, persisted or config change, and `gensrc` is untouched. The `AzureAuthFactory` signature changed inside `common/cpp`, which BE and Recycler each link into their own binary; they exchange no Azure object | | Parallel code paths updated | Only two paths reset a holder: [`s3_file_system.cpp:84`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L84) and [`cloud_storage_engine.cpp:204`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L204). The fix went into the first only: F-02. A sweep of every file-system binding site in both modes found no third. The Azure plus credential rejection is now the same on all three flat sources | | Special conditionals explained | The backend-index branch of `refresh` is commented, but its premise ("as the next BE start would build it") does not hold for bound rowsets: F-01. The `has_value()` check in `vault_process_error` handles a real input ([`cloud_storage_engine.cpp:134`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L134)) | | Test coverage (e2e, negative, unit) | The new tests close every test gap of the previous review. Still open: F-04, F-05, F-06. No test keeps a reader across a backend change (F-01) or refreshes a vault with another backend (F-02). 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) | The resource WARNING now names the id and the name, and the vault WARNING carries the failing status. Nothing logs that a resource was split, and the BE logs success: F-01 | | Transaction / persistence / EditLog replay | n/a. The meta service and FE are not changed | | Data write atomicity, crash safety | The writer logic is untouched. F-02 can place vault data in a dropped vault's bucket | | New FE↔BE variables on every send path | n/a, none added | | Performance and anti-patterns | No hot-path change. `refresh` adds one snapshot per push, and the include trimming in `be/src/util/s3_util.cpp` only reduces compile work | | Anything else | The CI `/review` run failed on model capacity (issue comment 6601595557), so the `code-review` status is still pending. The build hygiene gate passes at this head, run as a text scan | </details> <details> <summary>Considered and dismissed</summary> | # | Concern | Where | Why it is not an issue | |---|---|---|---| | D-01 | A rejected vault configuration changes which file system becomes `latest_fs`, or drops the vault | [`cloud_storage_engine.cpp:510`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L510) | The entry keeps its place ([`cloud_meta_mgr.cpp:1913-1916`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_meta_mgr.cpp#L1913-L1916)). Create returns the error and `get_filesystem(id)` stays null; refresh returns it and keeps the old file system. Both match a failed create or reset at the merge base, whose `S3Conf::get_s3_conf` never failed. `StorageVaultInfos` has no other consumer | | D-02 | The RFC 3986 flag set through `SDKOptions` behaves differently | [`s3_util.cpp:49-50`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/util/s3_util.cpp#L49-L50) | aws-sdk-cpp 1.11.221, the pinned version ([`vars.sh:391-394`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/thirdparty/vars.sh#L391-L394)), applies `httpOptions.compliantRfc3986Encoding` inside `InitAPI` (Aws.cpp, line 157). This is the only `InitAPI` in `be/src`, and `S3ClientFactoryTest.InitializesSdkWithRfc3986KeyEncoding` ran | | D-03 | Recycler builds its Azure client differently | [`s3_accessor.cpp:402-403`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/recycler/s3_accessor.cpp#L402-L403) | Same account, key and options as `git show 7acfd9bfc27:cloud/src/recycler/s3_accessor.cpp`. The init-time check of 28854079f3e is gone again (0225126bc43), and the branch is under `#ifdef USE_AZURE` | | D-04 | The Azure cache hash moved from crc32 to `std::hash` | [`azure_auth.cpp:61`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/common/cpp/obj-client/auth/azure/azure_auth.cpp#L61) | It is in-process only, and the client caches compare the full configuration ([`obj_client_cache.h:53`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_client_cache.h#L53)) | | D-05 | The merge resolution, or the 12 later master commits, break compile or hygiene | [`s3_util.cpp:20-34`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/util/s3_util.cpp#L20-L34) | Include lines only. COMPILE 1090348 and BE UT 1090345 are green. The later master commits touch no header and no object-storage file, the trial merge with `7866f5a36c0` is clean, and the removed symbols occur only in comments | | D-06 | An unknown `TObjStorageType` or `TCredProviderType` reaches `LOG_FATAL` / `__builtin_unreachable` | [`obj_storage_config_resolver.cpp:403`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L403) | Identical at the merge base (`git show 7acfd9bfc27:be/src/util/s3_util.cpp` lines 595-600, 658-660, 727-729). Not this PR | | D-07 | Unity collisions of the new internal names in `common/cpp` | [`CMakeLists.txt:27`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/common/cpp/CMakeLists.txt#L27) | `CommonCPP` sets no UNITY_BUILD property; only the BE pilot targets do | | D-08 | "Storage vaults cannot change their provider" is false even for ALTER | [`meta_service_resource.cpp:1093`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/cloud/src/meta-service/meta_service_resource.cpp#L1093) | ALTER rejects `provider`, and FE does not allow it in ALTER STORAGE VAULT. Only a reused id reaches the BE: F-02 | | D-09 | The split of F-01 misplaces cold data | [`tablet.cpp:2143`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/storage/tablet/tablet.cpp#L2143) | Cooldown, the output of cold data compaction and remote GC resolve the registry each time. Schema change writes through the input rowset's binding, whose endpoint and bucket are frozen under a storage policy. F-01 makes reads fail; it does not move data | | D-10 | `S3FileWriterTest` releases its S3 client after the SDK shuts down | [`s3_file_writer_test.cpp:332`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/test/io/fs/s3_file_writer_test.cpp#L332) | The upload pool drains first, and ExecEnv gets its old FileCacheFactory back before the owned one is destroyed. The ObjClientFactory cache that also holds the client is destroyed before `ShutdownAPI`. The `s3_fs.reset()` keeps the namespace-scope static from holding the last reference | | D-11 | Callers keep an `if (!built)` branch that can no longer fire | [`azure_client_provider.cpp:115`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/azure_client_provider.cpp#L115) | Harmless, and it becomes live with the next authentication method | | D-12 | A vault id reused across HDFS and S3 casts to the wrong file system type | [`cloud_storage_engine.cpp:202`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L202) | Identical at the merge base. Not this PR; it matters for the fix of F-02 | | D-13 | The Azure role_arn plus half-pair cell is also reachable from the meta service | [`obj_storage_config.cpp:143-151`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_storage_config.cpp#L143-L151) | True: ADD_OBJ_INFO, then `update_ak_sk`. Item 4 declares it: such a configuration could not authenticate before either | | D-14 | The first-sync connectivity check of a vault never runs | [`cloud_storage_engine.cpp:488`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/cloud/cloud_storage_engine.cpp#L488) | `first_sync_storage_vault` starts at `true` and the compare-and-swap expects `false`. This is the same at the merge base and not this PR; it is worth a separate issue | | D-15 | Earlier dismissals D-01 to D-17 of the af4964b review | [`s3_file_system.cpp:398`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L398) | Re-checked at this head. D-02 is now at [`s3_file_system.cpp:398`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/fs/s3_file_system.cpp#L398), D-08 at [`agent_server.cpp:145-146`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/agent/agent_server.cpp#L145-L146) and D-15 at [`azure_client_provider.cpp:119-120`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/azure_client_provider.cpp#L119-L120). The others name byte-identical code. D-16 (check_coverage red) is obsolete, because it is green | | D-16 | The reset log prints the update rather than the merged configuration that is published | [`obj_client_holder.cpp:55`](https://github.com/apache/doris/blob/f58598d426a9d97a110dba1f1e85f6279f0607b9/be/src/io/obj_storage/obj_client_holder.cpp#L55) | The same at the merge base (`git show 7acfd9bfc27:be/src/io/fs/s3_file_system.cpp` line 114 prints `conf`). Credentials are masked in both | </details> <details> <summary>Coverage and limits</summary> - Files reviewed in depth: the 31 files whose PR hunk changed since `af4964b` or is new (22 changed, 9 new) were read in full, or every hunk with its function and callers. Also read: the storage-side holders of a resource file system, the meta-service vault writers, and the FE resource, vault and filesystem-plugin code the findings depend on. None of these is changed by the PR. - Carried forward: 35 files are blob-identical to what the `af4964b` review read in full, at both bases and at both heads. Three more test files have the same PR hunk on a moved base and were checked against master's changes. - Not verified locally: nothing was built, no test was run, and nothing was executed on a cluster. The sequences in F-01 and F-02 were traced in the code. The only command run against the tree was the build hygiene gate, which is a text scan; the worktree was unchanged afterwards. CI evidence comes from TeamCity at this head: BE UT 1090345, Cloud UT 1090347, COMPILE 1090348 and the regression gates. - Evidence taken from outside the repository: aws-sdk-cpp tag 1.11.221 sources (Aws.cpp, URI.cpp, URI.h) fetched through the GitHub API; Azure SDK headers under the installed thirdparty; the TeamCity test lists of BE UT 1090345 and Cloud UT 1090347. - Inherited from earlier runs: `719de6a` (2026-10-26) and `af4964b` (2026-10-31). All 20 findings of `af4964b` are fixed at this head. Of its 17 dismissals, 16 were re-confirmed and carried forward, and one is obsolete. - Convergence: converged after 2 rounds. The verdict has been stable since round 1. Round 1 produced both Majors, so it did not converge on its own. Round 2 produced no new finding and only made the trigger of F-01 and the fix of F-02 more precise. </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]
