morningman commented on PR #69605: URL: https://github.com/apache/doris/pull/69605#issuecomment-6412792136
Review of commit `719de6ac81f0abe4889f8b43d8752edc4f927ebd`: **REQUEST_CHANGES** (1 Blocker, 1 Minor). **F-01 · Blocker: the new holder tests call an undefined helper** At [`obj_client_factory_test.cpp:281-287`](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/test/io/obj_storage/obj_client_factory_test.cpp#L281-L287), and again at [line 314](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/test/io/obj_storage/obj_client_factory_test.cpp#L314), the tests call `secret_key_of(...)`, but the helper has no declaration or definition. Repository symbol search finds only these three calls. The recursive source collection in [`be/test/CMakeLists.txt`](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/test/CMakeLists.txt#L24-L45) includes this file in the normal BE UT target, so this causes a compilation failure before any test can run. Please add the missing helper in the test's anonymous namespace, or access the typed S3 configuration directly. For example: ```cpp const std::string& secret_key_of(const ObjClientConfig& config) { return std::get<S3ClientConfig>(config.backend).auth.secret_key; } ``` This is a `functional-bug` regression: the merge base contains neither the new test file nor the unresolved calls. Please build and run the affected BE unit tests after fixing it. **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, [`ObjStorageConfigResolver::from_properties`](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L108-L110) still rejects an empty region before selecting the Azure backend. The [new test at lines 386-388](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/test/io/obj_storage/obj_client_factory_test.cpp#L386-L388) explicitly expects an Azure property map without `AWS_REGION` to fail. 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: > The typed Azure client factory no longer requires a region. Azure configurations converted from Thrift/PB metadata inherit this relaxation; property conversion retains its existing `AWS_REGION` requirement. 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. -- 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]
