wyxxxcat commented on PR #70119: URL: https://github.com/apache/doris/pull/70119#issuecomment-6621631441
English version of the review completed using the `doris-code-review` and `code-review` skills. Reviewed head: `0dc8be88f466e868adc14b28dd922aff07139f01`; pinned base: `6802f57005ee4f9e96caa609cc17ec264e368f6f`; merge base: `74b477d3c1431e10954add7698b6496ca370a59e`. Scope: 3 files, 100 additions and 1 deletion. **Standards: one P2 finding — Missing test coverage for the new implementation** [`s3_accessor.cpp:334–336`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/recycler/s3_accessor.cpp#L334-L336) constructs `GcsAccessor` only when `enable_gcs_batch_delete_object=false`, while the setting defaults to `true`. The existing [`S3AccessorTest.gcs`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/test/s3_accessor_test.cpp#L292-L331) does not set this flag, and no test directly constructs `GcsAccessor`. This PR does not modify any tests, so the default test configuration still exercises the old implementation and does not validate either new deletion implementation. This falls short of the repository requirement that kernel features have corresponding tests. Please cover both flag values and non-GCS providers, and add tests for pagination, expiration boundaries, missing objects, and error propagation through the new paths. **Spec: one P2 finding — Configuration name and default selection differ from the PR description** The PR description and release note state that `enable_gcs_accessor_for_batch_delete` is enabled by default, selects individual-object deletion by default, and falls back to `S3Accessor` when disabled. The implementation instead registers [`enable_gcs_batch_delete_object=true`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/common/config.h#L329), and [`create()`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/recycler/s3_accessor.cpp#L330-L343) constructs `S3Accessor` with that default, continuing to call `DeleteObjects`. Only setting the actual configuration key to `false` selects `GcsAccessor`. The documented key is not registered; startup configuration initialization iterates only over registered fields, so using that key will not switch implementations. Please align the configuration name, default, and selection condition. If the current implementation is intentional, update the PR description an d release note to document the correct way to enable it. No additional functional correctness issues were confirmed. The invocation chain was traced: Recycler → `S3Accessor::create()` → virtual calls through `StorageVaultAccessor` → GCS file-list/prefix deletion → existing `RateLimitedObjStorageClient` → S3 individual-object deletion. **Part 1.3 checkpoint conclusions** | Checkpoint | Conclusion | | --- | --- | | Goal and supporting evidence | Individual-object deletion implementations are present, but the default behavior differs from the description and tests do not explicitly select the new implementation. See the two findings above. | | Scope and reuse | Changes are focused on configuration and the accessor. Existing client, authentication, retry, and rate-limiting wrappers are reused. No additional maintainability issue was confirmed. | | Concurrency, shared variables, and locks | The non-`USE_LIBCPP` path uses parallel transform, writing to distinct result elements. Shared configuration and client usage follow existing patterns, and tasks complete before the method returns. No new lock-order or deadlock issue was found. | | Lifecycle and static initialization | Accessor and client ownership follow existing shared-ownership patterns. The iterator retains the client, and lambdas finish before the method returns. No new cross-translation-unit static initialization dependency was introduced. | | New configuration and dynamic updates | The new `CONF_Bool` is a startup setting read when creating an accessor; changes require a restart. The contract mismatch is covered by the Spec finding. | | Interface, format, and rolling-upgrade compatibility | No RPC, persisted format, or FE/BE protocol changes. The existing deletion implementation remains selectable. | | Parallel code paths | Both file-list and prefix deletion are overridden. Inherited `delete_prefix`, `delete_directory`, and `delete_all` dispatch to the new override. Non-GCS providers retain their existing selection. | | Special conditions | Path normalization, full object keys, the `mtime_s > expiration_time` filtering boundary, and successful handling of individual-object NOT_FOUND responses are preserved. Configuration selection is covered by the Spec finding. | | Test coverage | The Standards finding identifies the gap, particularly the lack of tests explicitly selecting the new implementation. | | Test result files | No test result files were added or modified. | | Observability | Existing client request metrics and error logs are reused, with prefix progress logs added. No additional clear blocker was identified. | | Transactions and persistence | KV/EditLog writes and recycling phase order are unchanged. No new state requires persistence. | | Data modification, atomicity, and crashes | Physical deletion remains retryable and may partially succeed before returning an error. Missing individual objects are treated as success, while existing callers retain control of metadata cleanup. Crash-injection validation was not performed. | | FE/BE variable propagation | Not applicable; no new variables cross the FE/BE boundary. | | Performance | File-list deletion adds an O(N) result array. Prefix deletion issues synchronous requests per object, and file-list deletion is serial under `USE_LIBCPP`. No performance tests were run, and no additional performance defect was confirmed. | | Other issues | No additional issues were confirmed. Continuing to scan after a failed deletion was explicitly excluded from this review's findings, as requested. | Validation consisted of source and call-chain inspection at the pinned revisions. Principal commands: ```text git diff 6802f57005ee4f9e96caa609cc17ec264e368f6f...0dc8be88f466e868adc14b28dd922aff07139f01 git log 6802f57005ee4f9e96caa609cc17ec264e368f6f..0dc8be88f466e868adc14b28dd922aff07139f01 --oneline rg -n 'GcsAccessor|enable_gcs_batch_delete_object' cloud/test cloud/conf ``` The diff contains the three files described above, the commit list contains only `0dc8be88f46`, and the final search returned no matches. As instructed, no builds, formatting checks, or unit tests were run. No production code was modified and no commit was created. These are static review conclusions, not evidence of passing runtime validation. Standards: one P2 finding. Spec: one P2 finding. Recommendation: align the configuration contract and add coverage for the new implementation before merging. -- 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]
