Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
2 similar comments
|
run buildall |
|
run buildall |
TPC-H: Total hot run time: 28494 ms |
TPC-DS: Total hot run time: 151321 ms |
ClickBench: Total hot run time: 24.36 s |
|
Review of commit F-01 · Blocker: the new holder tests call an undefined helper At Please add the missing helper in the test's anonymous namespace, or access the typed S3 configuration directly. For example: const std::string& secret_key_of(const ObjClientConfig& config) {
return std::get<S3ClientConfig>(config.backend).auth.secret_key;
}This is a F-02 · Minor: narrow the Azure region-removal claim in the PR description Behavior-change item 4 suggests that a flat Azure configuration no longer needs a region. However, This preserves the existing property-path behavior, so it is a wording issue rather than a runtime regression. Please limit the description to the paths that implement the relaxation, for example:
Validation: source review covered all 58 changed files and converged after two rounds. No builds or tests were run locally; the compilation finding is based on symbol lookup and the CMake source-collection rules. |
|
Architecture follow-up for commit Main relationships after the refactor 1. Consolidate the common creation flow and keep providers focused on SDK construction. The S3 provider and Azure provider both follow essentially the same sequence: They also call the factory's rate-limit helper, creating a dependency that goes Consider having the factory own the common creation flow and cache, while each provider only builds its backend client. The existing cache key already includes the backend, so it can distinguish clients for different backends. This would reduce duplication and make the dependency direction more direct. The tradeoff is sharing one short cache-lock critical section across the two backends. If separate caches are preferable, extracting a small common creation helper would still remove the duplicated sequence. A complex provider inheritance hierarchy is unnecessary here. 2. Keep one implementation of typed configuration validation. The resolver's validate_flat_fields and the factory's S3 validation repeat checks for the endpoint, region, credential combinations, and access/secret keys. It would help to distinguish:
The second group should have one implementation, always called by the factory and reused by the resolver when early validation is needed. Adding an authentication mode would then be less likely to leave two sets of rules inconsistent. Preserve existing compatibility rules during this cleanup. For example, Azure properties still require a region today; reorganizing validation should not silently change that behavior. 3. Extend compile-time variant checking to configuration merging and authentication. The factory's backend dispatch already uses
Adding a third backend or an Azure OAuth alternative would not necessarily expose every missing case at compile time. Some omissions could instead become a runtime Consider explicit per-alternative visitor overloads for these paths too. Pure configuration merging could also live in the configuration layer, so the holder's refresh logic does not need to depend on the input resolver. Both changes would make extension points more explicit. 4. Make the scope of shared connection options match their actual effect. ObjStorageConnectionOptions appears to apply to every backend. However, Azure construction receives only This inherits the old behavior and is not a new regression. The new model nevertheless gives callers a broader impression of support than the implementation provides. A small first step would be to document that these options currently apply only to S3. A later change could move them into S3 configuration or implement explicit Azure equivalents. Adding Azure support should be treated as a behavior change with corresponding tests. 5. As a lower-priority cleanup, align the remaining S3ClientFactory name with its responsibilities. The retained S3ClientFactory no longer creates object-storage clients. Its main responsibilities are SDK lifetime, AWS helpers, and CA handling; Azure still obtains CA information through it. At minimum, a more accurate name would avoid suggesting that the code now has two client factories. If CA handling is later extracted into a backend-neutral utility, keep SDK lifetime ownership explicit and preserve the guarantee that clients are destroyed before SDK shutdown. I would retain the typed configuration, centralized input conversion, and independent holder snapshots. Prioritizing points 1-3 would further reduce duplicated rules and dependency cycles, while concentrating the changes required to add a backend and making omissions easier for the compiler to detect. Validation was source-based; no builds or tests were run for this architecture analysis. |
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The holder tests in obj_client_factory_test call secret_key_of, but the helper was dropped together with an adjacent test when this PR was split from apache#69082, so the BE unit tests did not compile. The helper is restored. ### Release note None ### Check List (For Author) - Test: Unit Test (compiled and run in CI) - Behavior changed: No - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 28906 ms |
TPC-DS: Total hot run time: 151230 ms |
ClickBench: Total hot run time: 24.33 s |
…jClientFactory ### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: apply_rate_limit_policy was declared in obj_client_factory.h and defined in obj_client_factory.cpp. ObjClientFactory holds the client providers, and both providers called back into the factory to wrap a new client with the BE rate limiter, so the factory and the providers depended on each other. The policy (BeObjStorageRateLimitPolicy and apply_rate_limit_policy) moves to io/obj_storage/obj_client_rate_limit.{h,cpp}, and the providers include that instead. The dependency is now one way: factory -> providers -> rate limit policy. No behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (code moved without change; existing rate limit tests in s3_client_factory_test cover it, run in CI) - Behavior changed: No - Does this need documentation: No
|
run buildall |
…jClientFactory ### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: Same change as in apache#69605. apply_rate_limit_policy lived in ObjClientFactory while both client providers, which the factory holds, called back into it, so the factory and the providers depended on each other. The policy moves to io/obj_storage/obj_client_rate_limit.{h,cpp}; the dependency is now factory -> providers -> rate limit policy. No behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (code moved without change; run in CI) - Behavior changed: No - Does this need documentation: No
…layer ### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The typed configuration rules were split by where the old code lived rather than by which module owns them. Validation was implemented twice, as `validate_flat_fields` in the resolver and `validate_obj_client_backend` in the factory. The refresh merge `merge_update` lived in the resolver, so the holder depended on the input adapter, and it used `else` to mean Azure. Azure validation and construction read `AzureSharedKeyConfig` with `std::get`, so a new authentication alternative would fail at runtime with `bad_variant_access` instead of at compile time. - `validate_obj_client_config` and `merge_obj_client_config` are now part of the config layer (`obj_storage_config.{h,cpp}`) and are written as one overload per backend and per Azure authentication alternative, so a missing case fails to compile. The factory validates every request with it; the holder merges with it and no longer depends on the resolver. - The resolver keeps only the rules of FE properties that the typed configuration does not cover: a region for every provider, Azure included, and no provider-native credential for Azure. It then checks the converted configuration with the same `validate_obj_client_config`. - `AzureClientProvider` builds the credential options and the endpoint info through per-alternative overloads. - `ObjStorageConnectionOptions` documents that only S3 clients apply it today. Behavior is unchanged except that an invalid FE property map now fails at conversion instead of at the first client request, and when several fields are missing the reported one may differ. ### Release note None ### Check List (For Author) - Test: Unit Test (obj_client_factory_test covers typed Azure validation and the Azure provider-native credential rule on the property path; existing merge tests moved to merge_obj_client_config). Run in CI. - Behavior changed: No (see above) - Does this need documentation: No
ClickBench: Total hot run time: 24.33 s |
|
Review at head Code Review — PR #69605: refactor Build object storage clients from typed, per-backend configuration
SummaryThe PR replaces the flat Two Majors remain, and both are in the new code that applies a backend change. The fix for the old F-04 makes Findings at a glance
Status of the previous review's findingsThe previous review is this comment.
FindingsF-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
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, Why it happens. Commit b343fc6 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 ( 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
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 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 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 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
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 Why it happens. ALTER cannot change the provider of a vault ( When it bites. A cloud cluster in vault mode:
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 F-03 · Two new comments give a false reason why Recycler does not check the Azure key pair
// 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 ( Why it happens. Commit 0225126 removed the check again, rightly, and gave this reason. The real reason is that 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 F-04 · No test that runs in CI pins which value becomes the Azure SharedKey account and which the key
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 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 When it bites. The next change to this factory is the SAS / OAuth2 work the PR body announces. Suggested fix. Give F-05 ·
|
| 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. 2885407 adds a Recycler check and 0225126 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). 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 and cloud_storage_engine.cpp:204. 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) |
| 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 |
Considered and dismissed
| # | 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 |
The entry keeps its place (cloud_meta_mgr.cpp:1913-1916). 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 |
aws-sdk-cpp 1.11.221, the pinned version (vars.sh:391-394), 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 |
Same account, key and options as git show 7acfd9bfc27:cloud/src/recycler/s3_accessor.cpp. The init-time check of 2885407 is gone again (0225126), and the branch is under #ifdef USE_AZURE |
| D-04 | The Azure cache hash moved from crc32 to std::hash |
azure_auth.cpp:61 |
It is in-process only, and the client caches compare the full configuration (obj_client_cache.h:53) |
| D-05 | The merge resolution, or the 12 later master commits, break compile or hygiene | s3_util.cpp:20-34 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
Re-checked at this head. D-02 is now at s3_file_system.cpp:398, D-08 at agent_server.cpp:145-146 and D-15 at azure_client_provider.cpp:119-120. 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 |
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 |
Coverage and limits
- Files reviewed in depth: the 31 files whose PR hunk changed since
af4964bor 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
af4964breview 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) andaf4964b(2026-10-31). All 20 findings ofaf4964bare 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.
… writes to a reused vault id ### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: Two regressions in how a BE applies a change of backend (S3 family to Azure or back), plus the review's comment and test follow-ups. A cold-storage resource: FE allows `ALTER RESOURCE` to change the provider even under a storage policy, and `S3FileSystem::refresh` built a new file system for it. A remote rowset resolves its file system once and keeps it, so rowsets bound before the change stayed on the old holder: after a revert or a key rotation they kept failing with the old backend or the old key until BE restart, while FE and the BE log reported success. `refresh` now keeps the file system while the bucket and prefix stay, and rebinds its holder to a client built from the new configuration (`ObjClientHolder::rebind`), so every rowset follows, as before the refactor. It builds a new file system only when the bucket or prefix moves, which FE only allows without a storage policy. Every backend change logs a WARNING with the resource id and both configurations, masked. A storage vault: a vault's backend cannot be altered, but `drop_hdfs_vault` can drop an object vault and the meta service can hand its id to the next vault. The vault refresh only reset the holder, which refuses another backend, so loads for the new vault kept writing into the dropped vault's bucket; before the refactor they failed. `S3FileSystem::refresh_vault` now takes the holder out of service in that case (`ObjClientHolder::invalidate`). Every user shares the holder, including `PackedFileManager`, so all of them fail with their existing "no client" errors until a BE restart builds the new vault's file system. The invalidation is permanent: `reset`, `rebind` and `init` return its reason, and `_publish` checks it under the lock before the generation, so a refresh carrying an older configuration cannot bring the client back. The shared SDK client is left alone, since other holders may use it. Because the client can now become null at runtime: `S3FileWriter::_complete` takes it after waiting for the uploads, so that it no longer returns with upload callbacks still running and sees an invalidation during the wait, and `generate_presigned_url` checks it before its OSS branch, which would build a client of its own. Limits, accepted here: a call already sent cannot be recalled, so a PutObject that took the client may still succeed. Until the next vault sync (60s by default) a BE cannot know the id was reused. A reused id on the same backend or between HDFS and S3 is not detected. These need the fix in the meta service, which is handled separately. Review follow-ups: the comments on why Recycler does not check the Azure key pair now give the real reason (`S3Conf::from_obj_store_info` loads the pair complete or not at all; the meta service only requires it when an entry is created). New tests pin which value becomes the Azure account and which the key, in `AzureAuthFactory`, `AzureClientProvider` and Recycler's `S3Accessor`, without an account. `ObjClientFactoryTest` cases that count builds use unique endpoints, so they can be repeated in one process. ### Release note None ### Check List (For Author) - Test: Unit Test (holder rebind across S3, Azure and back with a kept reader, vendor change inside the S3 family, invalidation and overlapping reset, vault refresh, presigned URL, writer timing with sync points, Azure credential wiring in BE and Recycler; BE UT and cloud UT in CI) - Behavior changed: Yes. See the PR description, items 3 and 5. - Does this need documentation: No
|
run buildall |
… writes to a reused vault id Issue Number: None Related PR: apache#69605 Problem Summary: Two regressions in how a BE applies a change of backend (S3 family to Azure or back), plus the review's comment and test follow-ups. A cold-storage resource: FE allows `ALTER RESOURCE` to change the provider even under a storage policy, and `S3FileSystem::refresh` built a new file system for it. A remote rowset resolves its file system once and keeps it, so rowsets bound before the change stayed on the old holder: after a revert or a key rotation they kept failing with the old backend or the old key until BE restart, while FE and the BE log reported success. `refresh` now keeps the file system while the bucket and prefix stay, and rebinds its holder to a client built from the new configuration (`ObjClientHolder::rebind`), so every rowset follows, as before the refactor. It builds a new file system only when the bucket or prefix moves, which FE only allows without a storage policy. Every backend change logs a WARNING with the resource id and both configurations, masked. A storage vault: a vault's backend cannot be altered, but `drop_hdfs_vault` can drop an object vault and the meta service can hand its id to the next vault. The vault refresh only reset the holder, which refuses another backend, so loads for the new vault kept writing into the dropped vault's bucket; before the refactor they failed. `S3FileSystem::refresh_vault` now takes the holder out of service in that case (`ObjClientHolder::invalidate`). Every user shares the holder, including `PackedFileManager`, so all of them fail with their existing "no client" errors until a BE restart builds the new vault's file system. The invalidation is permanent: `reset`, `rebind` and `init` return its reason, and `_publish` checks it under the lock before the generation, so a refresh carrying an older configuration cannot bring the client back. The shared SDK client is left alone, since other holders may use it. Because the client can now become null at runtime: `S3FileWriter::_complete` takes it after waiting for the uploads, so that it no longer returns with upload callbacks still running and sees an invalidation during the wait, and `generate_presigned_url` checks it before its OSS branch, which would build a client of its own. Limits, accepted here: a call already sent cannot be recalled, so a PutObject that took the client may still succeed. Until the next vault sync (60s by default) a BE cannot know the id was reused. A reused id on the same backend or between HDFS and S3 is not detected. These need the fix in the meta service, which is handled separately. Review follow-ups: the comments on why Recycler does not check the Azure key pair now give the real reason (`S3Conf::from_obj_store_info` loads the pair complete or not at all; the meta service only requires it when an entry is created). New tests pin which value becomes the Azure account and which the key, in `AzureAuthFactory`, `AzureClientProvider` and Recycler's `S3Accessor`, without an account. `ObjClientFactoryTest` cases that count builds use unique endpoints, so they can be repeated in one process. None - Test: Unit Test (holder rebind across S3, Azure and back with a kept reader, vendor change inside the S3 family, invalidation and overlapping reset, vault refresh, presigned URL, writer timing with sync points, Azure credential wiring in BE and Recycler; BE UT and cloud UT in CI) - Behavior changed: Yes. See the PR description, items 3 and 5. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: `S3FileSystem::refresh` applies a refreshed cold-storage resource and can return a new file system, while `refresh_vault` applies a refreshed storage vault and never replaces the file system. The bare name read like a general version that a vault could use too, so it is now `refresh_resource`. No behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (rename; existing unit tests cover it, BE UT in CI) - Behavior changed: No - Does this need documentation: No
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The rename to refresh_resource pushed a comment line past the column limit. ### Release note None ### Check List (For Author) - Test: No need to test (comment only) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: `ObjStorageFileSystem::refresh` applies a refreshed cold-storage resource and can return a new file system, while `refresh_vault` applies a refreshed storage vault and never replaces the file system. The bare name read like a general version that a vault could use too, so it is now `refresh_resource`. No behavior change. ### Release note None ### Check List (For Author) - Test: No need to test (rename; existing unit tests cover it, BE UT in CI) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The holder invalidation tests only exercised synchronous close. An invalidated holder used to let async close return before an upload callback released its writer reference, leaving the destructor exposed to a use-after-free. Block a PutObject task with latches, invalidate before async close, and verify that close remains pending until the callback drains and then reports the error. Also update the resource refresh comment to describe retaining the holder. ### Release note None ### Check List (For Author) - Test: Added BE unit test; clang-format 16 and build hygiene passed. Local ASAN build and test execution follow this push; CI pending. - Behavior changed: No - Does this need documentation: No
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#69605 Problem Summary: The multipart invalidation test runs its hook after upload callbacks finish and before CompleteMultipartUpload, but its name and comment claimed invalidation happened during the wait. Rename the test and its object key and describe the actual timing. Test logic is unchanged. ### Release note None ### Check List (For Author) - Test: Repository clang-format 16 format/check scripts on an isolated copy of the changed file (identical to the worktree); git diff --check passed. Unit tests not run locally; CI requested after push. - Behavior changed: No - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 28681 ms |
TPC-DS: Total hot run time: 150883 ms |
ClickBench: Total hot run time: 24.15 s |
…berg WriteInfo (#70209) ### What problem does this PR solve? Issue Number: None Related PR: #69392, #69971 Problem Summary: **In short.** `doris_be_test` does not compile on master since #69971 was merged, so the BE UT of every PR fails with `BE BUILD FAIL`. This fixes the one test case that breaks it. **The problem.** #69392 replaced the `write_path`, `original_write_path` and `target_path` fields of `IPartitionWriterBase::WriteInfo` with one `IcebergOutputLocation location`, and updated the callers it knew about. #69971, merged after it, added `VParquetWriterTest.HiveAndIcebergParquetPageIndexOptions`, which still sets the old fields: ``` be/test/format/transformer/vparquet_writer_test.cpp:380:41: error: no matching constructor for initialization of 'VIcebergPartitionWriter' ``` Each PR built alone, so both were green. TeamCity's BE UT has failed this way for PRs merged with master `0eb5131db62` (for example build 1092024 of #69605). **How this PR fixes it.** The case builds its `WriteInfo` as the other Iceberg case in the same file does, `.location = IcebergOutputLocation(".", ".")`. Nothing else changes.
|
run beut |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
…rage-rename Bring in the latest apache#69605 (BE-1) and apply the renames of this PR to the code it added since: refresh_resource / refresh_vault, the invalidation tests, and the master changes it carries (S3FileReaderTest, the peer race skip counter, the split spill writers).
…mment ### What problem does this PR solve? Issue Number: None Related PR: apache#69605, apache#69082 Problem Summary: The bulk rename of S3ClientFactory to AwsSdkManager also rewrote a comment in obj_storage_config.cpp that names the function the config checks came from, so it read "the former AwsSdkManager::create". That function was S3ClientFactory::create; AwsSdkManager never had a create method. Restore the original name. A scan of comments that name a method of AwsSdkManager or the ObjStorage* classes found no other method that does not exist. ### Release note None ### Check List (For Author) - Test: No need to test (comment only) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#69605 Problem Summary: The typed configuration omitted the bucket from the S3 client cache key. Buckets with otherwise identical settings would share a client and its connection pool. Carry the bucket from legacy inputs through cache lookup and reset. S3 requests still supply their target bucket individually. ### Release note None ### Check List (For Author) - Test: 69 ASAN BE unit tests passed across ObjClientFactoryTest, ObjStorageUriS3RulesTest and S3ClientFactoryTest. Header hygiene, formatting and clang-tidy passed. - Behavior changed: No (preserves the pre-refactor S3 cache behavior) - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#69082, apache#69605 Problem Summary: Keep the integration branch aligned with the split BE refactor. Include the bucket supplied by the legacy configuration sources in the S3 client cache key and preserve it through refresh. This keeps separate connection pools for otherwise identical configurations that name different buckets. ### Release note None ### Check List (For Author) - Test: FE and ASAN BE builds passed. A local two-bucket S3 regression passed with cold and warm caches. BE logs confirmed two client builds, and SQL and an independent S3 client verified the exported data. Header hygiene, formatting and clang-tidy passed. - Behavior changed: No (preserves the pre-refactor S3 cache behavior) - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#69605 Problem Summary: Public URL generation assumed an S3 client whenever an endpoint matched the OSS private endpoint suffix. A custom Azure endpoint with that suffix would fail before signing. Rewrite the endpoint in a copy of the configured backend so its own client signs the public URL. ### Release note None ### Check List (For Author) - Test: FE and ASAN BE builds passed. All 74 selected BE unit tests passed with Azure enabled, including public and private signing. Header hygiene, clang-format 16 and clang-tidy passed. - Behavior changed: No (restores pre-refactor public URL generation). - Does this need documentation: No
What problem does this PR solve?
Issue Number: None
Related PR: #69082
Problem Summary: Every BE object storage entry (FE properties, cold-storage resources, storage vaults) produced one flat
S3Conf/S3ClientConfthat carried all providers in a single shape: Azure reused ak/sk/bucket as account/key/container, while region, role_arn and the credential provider type only meant something for S3.S3ClientFactory::createthen branched on the provider to build an S3 or an Azure client, andObjClientHoldermerged credential refreshes field by field into that flat struct. A backend-specific field could leak into the other backend's identity or validation (for example, an Azure configuration still needed a region), and adding an Azure authentication method would have meant more flat fields that S3 must ignore.This is the first step of splitting #69082 (BE object storage refactor and native Azure SAS / OAuth2 access). It only restructures the BE; no FE change, no Thrift change, and Azure still supports only SharedKey.
io/obj_storage/obj_storage_config.{h,cpp}).io::ObjClientConfigholds the shared connection options, the client usage (internal storage / external access) and exactly one backend,std::variant<S3ClientConfig, AzureClientConfig>. The Azure authentication is theAzureAuthConfigshared with Recycler (see below).io::ObjStorageConfig(bucket, prefix, client) replacesS3Conf;S3ConfandS3ClientConfare removed. The rules of the typed configuration live in this layer, written per backend and per authentication throughstd::visit, so a missing branch fails to compile:validate_obj_client_config(whether a configuration is valid) andmerge_obj_client_config(how a refreshed resource or vault configuration merges into the current one, with the same field rules as the flat merge).ObjStorageConfigResolver::from_properties(formerlyS3ClientFactory::convert_properties_to_s3_conf),from_storage_paramandfrom_object_store_info(formerlyS3Conf::get_s3_conf) turn FE properties,TS3StorageParamandObjectStoreInfoPBinto the typed configuration, and all three returnResult. The flat field mapping is ported unchanged. A provider-native credential next to provider AZURE, which only the S3 backend has a field for, is rejected with InvalidArgument for all three sources. The resolver keeps only the rule of its own input (FE properties needAWS_REGION) and otherwise relies on the config layer. AI embed, Kinesis and the file cache microbench build the typed configuration themselves.ObjClientFactoryvalidates every request withvalidate_obj_client_config(the checksS3ClientFactory::createapplied), owns the test hookset_client_creator_for_test, and dispatches throughstd::visit.S3ClientProviderandAzureClientProviderbuild and cache their clients outside the cache lock. The BE rate limit policy is its own module (obj_client_rate_limit) shared by both providers.S3ClientFactorykeeps only the AWS SDK lifetime and the AWS helpers (client configuration, credentials provider, CA path), which Kinesis also uses.AzureAuthFactory, but BE kept its own authentication variant and converted it back into the flatAzureCredentialOptions(a type enum plus fields). The model now lives once in common, next to the GCP credential, undercpp/obj-client/auth/azure/:azure_auth.h(SDK-free, always built) definesAzureSharedKeyConfigand theAzureAuthConfigvariant, with the rules every method shares (validate_azure_authfor required fields,hash_combine_azure_auth,azure_auth_type_namewithout secrets).AzureAuthFactorytakesAzureAuthConfigdirectly, with onestd::visitbranch per method, andAzureCredentialType/AzureCredentialOptionsare removed; transport, CA, retries and policies still come from each process throughBlobClientOptions. The storage account name stays outside the variant, because it names the storage target. BE'sAzureClientConfigholds the common config, BE has no Azure authentication logic of its own, and BE checks its inputs withvalidate_azure_auth. Recycler builds anAzureSharedKeyConfigfrom its storage metadata and passes it to the same factory; it does not call the check, becauseS3Conf::from_obj_store_infoloads the ak/sk pair complete or not at all (the meta service requires the pair when an Azure entry is created, but not on every later update), so Recycler builds the client exactly as before. A new method (SAS, OAuth2, Managed Identity) is then added once in common, and each entry only adapts its input or rejects the method.ObjClientHoldermoves toio/obj_storage/obj_client_holder.{h,cpp}and owns an immutableObjClientState(config, client, generation) that it replaces as a whole, so a caller that reads both from onestate()snapshot never pairs a new config with an old client. A reset merges withmerge_obj_client_configand builds throughObjClientFactory.ObjStorageUri::parse_s3replacesS3URIwith the same rules.FileFactoryresolves FILE_S3 locations through one function for the reader,create_fsandcreate_file_writer.S3AuthConfig::credentialcarries the provider-nativeCredentialConfig, the resolver parses and converts it for all three sources,ObjClientFactoryvalidates it on every request, andS3ClientProviderbuilds the client throughmake_s3_clientwith the GCS V4 signed URL generator. A reset takes the vendor and credential from the update, as the flat merge does.SDKOptions::httpOptionsat SDK initialization instead of in every reader and writer constructor. The reader no longer uses AWS types for HTTP 429.Behavior changes:
Abortedinstead of publishing on top of a state it was not merged with, so overlapping resets cannot both win; the caller retries with its next refresh. The check detects overlap, not freshness: a reset carrying an older configuration can still win, as before, and the next refresh replaces it.ObjClientHolder::resetno longer accepts another backend (InvalidArgument), since its merge keeps the storage target. FE still allowsALTER RESOURCEto change the provider of a cold-storage resource, also under a storage policy, andS3FileSystem::refresh_resourcehandles that: while the bucket and prefix stay, it rebinds the existing holder to a client built from the new configuration (ObjClientHolder::rebind), and a rowset that already resolved the file system follows, including after a revert or a key rotation. This matches the merge base under a storage policy, where FE freezes the endpoint, region, bucket and root path: reads fail on access and recover when the change is reverted. Without a storage policy the new configuration applies as it is, the same result as after a restart, where the merge base kept the old endpoint and failed; a moved bucket or prefix builds a new file system with the same id. Each change of backend logs a WARNING with the resource id and both configurations, masked. It is not a supported way to move a resource; rejecting it in FE is a separate follow-up.from_properties) keeps its existingAWS_REGIONrequirement. The Azure account name and key are checked as a pair without the role_arn exemption of the flat ak/sk rule, because Azure has no role authentication; such a configuration could not authenticate before either, and now fails when the client is built.drop_hdfs_vaultaccepts an object vault, and a new id is the largest existing one plus one). When a vault sync finds another backend under an id,S3FileSystem::refresh_vaulttakes the holder out of service for good (ObjClientHolder::invalidate): readers, writers andPackedFileManager, which all share it, fail until a BE restart builds the new vault's file system, as at the merge base, instead of writing into the dropped vault's bucket. Limits: a call already sent is not recalled, so a PutObject that took the client may still succeed; until the next vault sync (60s by default) a BE cannot know the id was reused; and a reused id on the same backend, or between HDFS and S3, is not detected. Those need the fix in the meta service, which is separate from this PR.Release note
None
Check List (For Author)