wyxxxcat commented on PR #70119: URL: https://github.com/apache/doris/pull/70119#issuecomment-6621592785
按 `doris-code-review` / `code-review` 技能完成双轴审查。 审查提交：`0dc8be88f466e868adc14b28dd922aff07139f01`；固定 base：`6802f57005ee4f9e96caa609cc17ec264e368f6f`；merge-base：`74b477d3c1431e10954add7698b6496ca370a59e`。范围为 3 个文件，新增 100 行、删除 1 行。 **Standards：1 项 P2，缺少新分支的测试覆盖** [`s3_accessor.cpp:334–336`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/recycler/s3_accessor.cpp#L334-L336) 仅在 `enable_gcs_batch_delete_object=false` 时构造 `GcsAccessor`，而该配置默认是 `true`。现有 [`S3AccessorTest.gcs`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/test/s3_accessor_test.cpp#L292-L331) 没有设置这个开关；测试目录中也没有直接构造 `GcsAccessor` 的用例。本次未修改测试，因此默认测试配置仍走旧实现，无法验证新增的两个删除实现。这不符合仓库要求内核功能配套测试的规范。建议覆盖开关两种取值及非 GCS provider，并为新路径补充分页、过期时间边界、对象不存在和错误返回的测试。 **Spec：1 项 P2，配置名及默认路由与 PR 声明不一致** PR 说明和 release note 声明 `enable_gcs_accessor_for_batch_delete` 默认启用，默认使用单对象删除，关闭后回退到 `S3Accessor`。实际注册的是 [`enable_gcs_batch_delete_object=true`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/common/config.h#L329)，且 [`create()`](https://github.com/apache/doris/blob/0dc8be88f466e868adc14b28dd922aff07139f01/cloud/src/recycler/s3_accessor.cpp#L330-L343) 在默认值下构造 `S3Accessor`，仍调用 `DeleteObjects`；设置实际配置为 `false` 才切换到 `GcsAccessor`。说明中的配置名没有注册，启动配置初始化只遍历已注册字段，按说明填写该键不会切换实现。请统一配置名称、默认值和选择条件；如果当前代码行为是有意保留的契约，应修正 PR 说明和 release note，明确正确的启用方法。 未发现其他已确认的功能正确性问题。调用链已核对：Recycler → `S3Accessor::create()` → `StorageVaultAccessor` 虚调用 → GCS 文件列表／前缀删除 → 现有 `RateLimitedObjStorageClient` → S3 单对象删除。 **Part 1.3 检查结论** | 检查项 | 结论 | | --- | --- | | 目标及证明 | 单对象删除实现已加入；默认行为与说明不符，且缺少新分支测试，见上述两项。 | | 修改范围及复用 | 改动集中于配置和 accessor；复用客户端、认证、重试和限流包装，无其他已确认的维护性问题。 | | 并发、临界变量及锁 | 非 `USE_LIBCPP` 路径使用并行 transform；结果写入不同元素，共享配置及客户端沿用既有使用方式；返回前完成任务，未发现新增锁顺序或死锁问题。 | | 生命周期及静态初始化 | accessor 和客户端沿用共享所有权；迭代器持有客户端，lambda 在返回前执行完；未新增跨翻译单元静态初始化依赖。 | | 新配置及动态生效 | 新增 `CONF_Bool`，属于启动配置，创建 accessor 时读取，修改需要重启；契约不一致见 Spec。 | | 接口、格式及滚动升级兼容性 | 未修改 RPC、持久化格式或 FE/BE 协议；旧删除实现仍可选择。 | | 平行代码路径 | 文件列表和前缀入口均有覆盖；继承的 `delete_prefix`、`delete_directory`、`delete_all` 会虚派发到新实现，非 GCS 路径保持原选择。 | | 特殊条件判断 | 路径规范化、完整对象 key、过期过滤的 `mtime_s > expiration_time` 边界及单对象 NOT_FOUND 成功语义均保留；配置判断见 Spec。 | | 测试覆盖 | 存在上述 Standards 缺口，尤其缺少明确选择新实现的用例。 | | 测试结果文件 | 本次未新增或修改测试结果文件。 | | 可观测性 | 复用客户端请求指标及错误日志，增加前缀进度日志；未发现其他明确阻断项。 | | 事务及持久化 | 未改动 KV/EditLog 写入或回收阶段顺序，未新增需要持久化的状态。 | | 数据修改、原子性及崩溃 | 仍是可重试的物理删除，允许部分成功后返回错误；单对象不存在按成功处理，元数据清理仍由原调用方控制；未做崩溃注入验证。 | | FE/BE 变量传递 | 不适用，无新增跨 FE/BE 变量。 | | 性能 | 文件列表增加 O(N) 结果数组；前缀按对象同步删除，文件列表在 `USE_LIBCPP` 下串行。未做性能测试，无额外已确认的性能缺陷结论。 | | 其他问题 | 未确认其他问题；“删除失败后继续扫描”按本次审查范围排除，不计入结论。 | 验证采用固定版本的源码和调用链检查，主要命令： ```text git diff 6802f57005ee4f9e96caa609cc17ec264e368f6f...0dc8be88f466e868adc14b28dd922aff07139f01 git log 6802f57005ee4f9e96caa609cc17ec264e368f6f..0dc8be88f466e868adc14b28dd922aff07139f01 --oneline rg -n 'GcsAccessor|enable_gcs_batch_delete_object' cloud/test cloud/conf ``` diff 为上述 3 个文件，commit 列表仅包含 `0dc8be88f46`，最后一条检索无匹配。遵照要求，未运行编译、格式检查或单元测试；未修改业务代码、未提交 commit。以上属于静态审查结论，不代表运行验证通过。 结论：Standards 1 项 P2，Spec 1 项 P2；建议统一配置契约并补齐新分支测试后再合并。 -- 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]
