wyxxxcat commented on PR #70119: URL: https://github.com/apache/doris/pull/70119#issuecomment-6621683624
Re-review using the `doris-code-review` / `code-review` workflow against the updated PR description. Reviewed head: `0dc8be88f466e868adc14b28dd922aff07139f01`; pinned base: `6802f57005ee4f9e96caa609cc17ec264e368f6f`; merge base: `74b477d3c1431e10954add7698b6496ca370a59e`. The code is unchanged: 3 files, 100 additions and 1 deletion. This report supersedes my previous review conclusions. Testing and test coverage are outside the requested scope, so the earlier missing-test finding is withdrawn from this review. Continuing to scan after a failed deletion remains excluded as previously requested. **Standards: no findings within the requested scope.** **Spec: one P2 finding — The documented default and switch behavior are still reversed.** The configuration name now matches the code, resolving that part of the previous finding. However, the description still states that the enabled default selects `GcsAccessor`, and the release note says setting `enable_gcs_batch_delete_object=false` retains the generic S3 accessor. The behavior checklist also still describes `GcsAccessor` as the default with an opt-out configuration. The implementation does the opposite: [`config.h:329`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/common/config.h#L329) defaults the flag to `true`, and [`s3_accessor.cpp:334–339`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/recycler/s3_accessor.cpp#L334-L339) selects `GcsAccessor` only when it is `false`. | Configuration for a GCS provider | Actual accessor | Deletion mode | | --- | --- | --- | | `enable_gcs_batch_delete_object=true` (default) | `S3Accessor` | Batch deletion through `DeleteObjects` | | `enable_gcs_batch_delete_object=false` | `GcsAccessor` | Individual-object deletion through `DeleteObject` | An operator following the release note to retain batch deletion would instead enable individual-object deletion. If the code reflects the intended behavior, update the description, release note, and behavior checklist accordingly. Suggested wording: > GCS recycling retains generic S3 batch deletion by default. Set `enable_gcs_batch_delete_object=false` at startup to use `GcsAccessor` for individual-object deletion of file lists and prefixes. No additional functional correctness issues or scope changes were confirmed within the requested scope. The checked invocation chain remains: Recycler → `S3Accessor::create()` → virtual accessor calls → file-list/prefix deletion → existing rate-limited object client → object-storage deletion. **Part 1.3 checkpoint conclusions** | Checkpoint | Conclusion | | --- | --- | | Goal and evidence | Individual-object deletion is implemented and selectable; the description still reverses its default and selection condition. Runtime testing is outside scope. | | Scope and reuse | Focused three-file change; existing client, authentication, retries, and rate limiting are reused. | | Concurrency and locking | Parallel file-list deletion writes distinct result elements and completes before returning; no new lock-order or deadlock issue was confirmed. | | Lifecycle and static initialization | Existing shared ownership is retained; iterator and lambda lifetimes cover their operations. No new cross-translation-unit initialization dependency. | | Configuration and dynamic updates | Immutable startup flag, read when constructing the accessor; behavior is shown in the table above. | | Compatibility | No persisted format or FE/BE protocol changes; the existing accessor remains available. | | Parallel paths | File-list and prefix deletion are overridden. Inherited prefix, directory, and whole-vault entry points dispatch to the override. Non-GCS selection is unchanged. | | Special conditions | Path normalization, full listed keys, expiration filtering, and individual-object NOT_FOUND success handling are preserved. | | Test coverage | Excluded from this review as requested; no finding raised. | | Test result files | Outside the requested scope; no result files changed in the diff. | | Observability | Existing request metrics and error logs are reused; prefix progress logging is present. No additional confirmed blocker. | | Transactions and persistence | No changes to KV/EditLog writes, recycling phases, or persisted state. | | Data modification, atomicity, and crashes | Physical deletion remains retryable, with errors returned to existing callers that control metadata cleanup. No new crash-related defect was confirmed by source inspection. | | FE/BE variable propagation | Not applicable. | | Performance | Prefix deletion is synchronous per object; file-list deletion uses an O(N) result array and is serial under `USE_LIBCPP`. No additional performance defect was confirmed; no benchmark was run. | | Other issues | No additional confirmed findings within scope. The previously excluded scan-after-error concern remains excluded. | Validation: re-read the updated PR description, confirmed the unchanged head and file list, and inspected the full pinned diff, factory selection, and deletion implementations. The pinned commit range still contains only `0dc8be88f46`. No builds, formatting checks, or tests were run; no code was changed or committed. Standards: 0 findings. Spec: 1 P2 finding concerning the remaining description/release-note mismatch. -- 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]
