[refactor](storage) Unify BE and Recycler object clients - #66350
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
f30abce to
dc25d3e
Compare
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
1fab2ee to
4ea09ca
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static-only review of the full authoritative diff found six issues that should be addressed before merge (five P1, one P2).
Review-cycle status: incomplete after the three-round cap. Both normal agents returned NO_NEW_VALUABLE_FINDINGS in Round 3, but the risk-focused agent found a final FE-side scope correction that was independently verified and merged into the token round-trip comment; the review contract does not permit a fourth round. All currently known candidates are nevertheless adjudicated and included below.
Critical checkpoint conclusions:
- Data correctness: failed. Session-token credentials are dropped/staled across FE DDL and meta-service paths, and Recycler exists status mapping can turn real provider failures into false not-found results.
- Concurrency and lifecycle: delete-task ownership, executor waiting, batch clamping, and error propagation are sound; request admission during recursive deletion is not.
- Configuration and dynamic behavior: Recycler rate limiting and PUT fault injection are bypassed for the actual recursive-delete SDK requests; AWS provider precedence, refresh-capable providers, and client cache identity otherwise remain compatible.
- Compatibility and rolling behavior: the optional protobuf field is wire-compatible, but the Recycler 0/1/negative adapter contract and GCS iterator migration are broken.
- Parallel paths: BE/Recycler and S3/Azure/GCS paths were traced; the GCS path has an unconditional compile failure and Recycler differs from the preserved BE admission behavior.
- Tests and validation: no builds or tests were run, as required by the review prompt. Existing S3 accessor tests still require 1 for not-found, and there is no end-to-end token persistence/redaction/rotation coverage; the GCS compile error is statically evident.
- Observability and security: the session token lacks SK-equivalent encryption/log/display handling, and successful S3 writes now log at INFO on the hot path. This is credential-secret handling within authenticated control paths; no unsupported cross-tenant vulnerability claim is made.
- Persistence and recovery: token-bearing vault/stage records can either lose the token or retain it plaintext, so persistence round trips are not safe.
- Performance: recursive deletion can evade Recycler request controls, while per-write INFO logging adds log I/O proportional to storage QPS; page and provider batch limits themselves are sound.
User focus: review_focus.txt supplied no additional focus, so the entire PR was reviewed without narrowing scope.
FE UT Coverage ReportIncrement line coverage |
92a3812 to
3f0e01e
Compare
|
run buildall |
2 similar comments
|
run buildall |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: two P2 test regressions leave important failure paths able to pass unverified.
Findings:
- The migrated S3/role/Azure iterator loops do not assert the terminal iterator response, so an expected-empty post-delete check can pass when the list request actually failed.
- The replacement recursive-delete tests exercise only the shared facade's sequential fallback, while Recycler production always uses the parallel
SyncExecutorpath and its cancellation/unfinished-task handling.
Checkpoint conclusions:
- Goal and scope: reviewed all 63 authoritative changed paths and the full object-client unification across CommonCPP, BE, Cloud Recycler, credentials, build wiring, adapters, and tests. No additional user-provided focus was supplied, so the full PR remained the focus.
- Functional correctness and error handling: request-level GET/PUT admission, read-byte settlement, S3/Azure page and delete contracts, continuation tokens, prefix/key conversion, NOT_FOUND handling, and recursive-delete error propagation are internally consistent on this head. Known production concerns already raised in live threads were not duplicated.
- Concurrency and lifecycle: recursive tasks retain shared backend/policy ownership, batch limits are bounded, and executor failures are surfaced. The missing production-executor regression coverage is called out inline.
- Configuration, credentials, and compatibility: V1/V2 AWS provider selection, BE session tokens, role/external-ID/CA propagation, Azure shared-key construction,
Resultmigration, namespace aliases, CommonCPP linkage, and Azure-on/off gating were checked statically with no additional substantiated defect. - Persistence and data mutation: no FE/BE transaction or metadata protocol is introduced here; object deletion remains provider-batched and reports list/delete failures rather than clean partial success.
- Performance and observability: one admission maps to each provider list page/delete batch, provider batch caps remain 1000 for S3 and 256 for Azure, and latency/failure instrumentation plus slow-request logging remain present.
- Tests and verification: the two inline comments identify the remaining coverage/oracle gaps. Per the authoritative review contract, this was a static-only review; no local build or test command was run.
- Completion: two review rounds converged on this exact two-comment set, all candidates were accepted, deduplicated, or dismissed with evidence, and the live head/base still match the authoritative bundle.
c626cf6 to
a46b0f7
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
6b96fb0 to
13f9891
Compare
|
run buildall |
|
/review |
|
run buildall |
|
/review |
1. What does this PR do?
BE and Cloud Recycler previously maintained separate object-storage abstractions and separate S3/Azure implementations. Although both sides called the same cloud-provider SDKs, credential construction, error conversion, metrics, pagination, batch deletion, and compatibility behavior were duplicated and could evolve differently.
This PR consolidates the implementation under
common/cpp/clientand exposes oneObjStorageClientfacade to upper layers:ObjStorageClientowns backend-independent orchestration and is the only complete client used by BE and Recycler call sites.ObjStorageRateLimitPolicykeeps BE- and Recycler-specific admission behavior injectable without coupling common code to either environment.ObjStorageBackendis the storage implementation boundary, implemented byS3ObjStorageBackendandAzureObjStorageBackend.Before the refactor, BE and Recycler reached the cloud SDKs through parallel stacks:
After the refactor, BE and Recycler stay on the left and right while the shared facade and backend components are centered below them:
This boundary prevents a raw backend from being used as the complete client and accidentally bypassing runtime policy. Backend code only implements cloud mechanics; common orchestration and policy dispatch remain in the facade.
2. How are the different behaviors unified?
doris::io::ObjStorageClientand eager list resultsdoris::ObjStorageClientfacade and one set of request/response types.doris::ioaliases keep BE call sites source-compatible; Recycler adapters preserve its integer-facing API.S3ObjStorageBackendandAzureObjStorageBackendare shared by both callers.ObjStorageRateLimitPolicy; the facade performs admission immediately before backend work. Each list page and each backend-sized delete batch is admitted independently.ObjectStorageResponseconsistently carries a Doris status code, HTTP code, and request ID, while adapters preserve caller-facing behavior such as the Recycler 0/1/negativeexistscontract.ObjStorageClient::list_objectsreturns one fixed-sizeObjectStorageListPage. One Client call performs one GET admission, one Backend call, and one SDK request. The upperObjectListIteratorowns the continuation token and requests the next page only after its cached page is consumed.END_OF_FILEis an internal upper-iterator sentinel andnext()converts it to a successful empty result. BackendNOT_FOUNDremains a real error.NoSuchKeyhandling existed in the BE pathNoSuchKey-as-empty behavior once for both callers.SyncExecutor; BE uses the synchronous fallback.AwsCredentialFactoryimplements static credentials, default provider chains, role ARN, and external ID once while callers retain their prior empty-credential behavior.AzureAuthFactorycreates the container client and shared-key credential for both; BE TLS diagnostic context remains attached to Azure errors.client_bvar::ScopedLatencytimer and common failure metrics. The previous BE 5-second slow-request logging behavior is retained for S3 uploads.3. Design boundaries and follow-ups
std::shared_ptr<ObjStorageClient>; they do not storeObjStorageBackenddirectly.ObjectListIteratorperforms lazy iteration by repeatedly calling the one-page Client API. Therefore each requested page has exactly one facade admission and one SDK request; reading objects already cached in that page performs no network request.delete_objectscall may split input according to backend capability, but each resulting backend batch acquires its own PUT admission before issuing one SDK request.SyncExecutor.git diff --checkpassed.