wenzhenghu commented on PR #67750: URL: https://github.com/apache/doris/pull/67750#issuecomment-5611036989
> 迁移自 @xuchenhao 在内部镜像 PR（HYDCP/hy-doris#103）上的 review，原文见[内部评论](https://github.com/HYDCP/hy-doris/pull/103#issuecomment-5611002429)。以下内容保持原作者结论，仅对内部引用做了链接保留。 ## Review：LGTM（代码审查结论，验证边界见下）——@xuchenhao 审查全部 2 个文件（+170/-18）。独立子 agent review 后由主审复核，已阅读 PR 描述、上游 issue #67303 与已有讨论。 **未发现本次改动引入的确定缺陷。** 三条过期删除路径都先在读锁内记录候选，再在逐项写锁内确认 `RecycleInfo` 身份、时间戳数值和过期性；校验失败时，流程在 metadata/cache/resource cleanup 与 erase journal 之前跳过。recover 后再次 recycle 会创建新的 `Recycle*Info`，因此即便 metadata ID 相同，也不能被旧候选擦除。 PR 明确排除的 same-name database cascade 长持锁问题仍按上游 issue 跟踪，不作为本次漏修重报，也不据此宣称整个 recycle-bin 的所有并发问题已解决。 ### 对已有评论的一处技术纠正（非代码缺陷） [前面的 review](https://github.com/apache/doris/pull/67750#issuecomment-5611031982) 将时间戳检查解释为「装箱 Long 引用相等」，这不符合实际代码：`ExpiredCandidate.recycleTime` 是 primitive `long`，所以 [helper 中的比较](https://github.com/apache/doris/pull/67750/files#diff-d47c1b5f8f2af5e7152aa2bbcaf7c2d04f3f0d81c0f6e5aa6eb1c7860b6ffb5fR318-R323) 会将 map 返回的 Long 拆箱，做**数值比较**，不是 Long 对象身份比较。 换代由前一项 `recycleInfoMap.get(id) == candidate.recycleInfo` 的引用身份检查保证，与 Long 缓存/重新装箱无关。因此不应添加「必须依赖不同 Long box、改成数值 equals 会破坏换代保护」这样的注释。主审已用 Java 11 独立最小程序验证：两个不同 Long 对象但相同数值，与 primitive long 比较为 true；字节码为 `longValue()` + `lcmp`。这只是 Java 语义诊断，不是 Doris FE UT。 ### 关键检查点 | 检查点 | 结论 | |---|---| | 目标与证明 | 修复过期扫描到写锁删除之间的 ABA；三个新增测试分别覆盖 db/table/partition 的精确换代窗口。 | | 最小范围与复用 | 仅候选快照、共用验证 helper 和对应测试，无无关重构。 | | 并发与锁 | DDL recover/recycle 与 daemon 共享既有 recycle-bin 锁；正常在线换代不能插入验证到副作用之间；未增加锁或锁序边。既有清理/日志等待仍在原写锁范围内，本 PR 未承诺消除它们。 | | 生命周期 | 候选仅活于当前 sweep；测试通过 finally 放行 latch、等待 executor 并恢复锁，无新增全局生命周期。 | | 配置 | 无新增配置；重检读取当前 retention 配置，并保留 sweep 固定时间边界。 | | 兼容性 | 无持久化字段、image/journal/RPC 格式变更，未改变回放协议。 | | 平行路径 | db/table/partition 三条过期路径一致使用 helper；同名数量清理与 replay 路径已核对，未把其既有行为泛化成本次新增问题。 | | 条件正确性 | 新代、时间戳变更或不再过期均跳过；条目已被移除时先由 info 身份检查短路，不再进入时间戳拆箱。 | | 测试覆盖 | latch 在首次扫描 readUnlock 之后暂停，非 sleep 猜测；换代使用公开 recover/recycle 方法。父对象未回收，不会被 isRecycleXxx 的父级 OR 条件掩盖误删。 | | 可观测性 | 跳过不会写成功 erase 日志或 journal；沿用现有日志，无必须新增指标的证据。 | | 事务与持久化 | 三条 erase journal 均在校验之后；既有 OP_ERASE_DB/TABLE/PARTITION 回放保持不变。未实际验证故障切换。 | | 数据修改与异常 | 校验失败无删除副作用，成功路径保留既有锁内修改与异常处理；未发现新增崩溃一致性路径。 | | FE/BE 变量 | 无新增跨端变量或协议字段。 | | 性能 | 每个过期对象增加一个局部候选；验证 O(1)，扫描复杂度不变，未重新扩大逐项删除锁范围。 | | 其它风险 | 未发现新的阻塞项；既有上游问题和下面的覆盖缺口不等同于本次代码缺陷。 | ### 验证与非阻塞覆盖建议 - 本次实际执行：完整 `git diff --check` 通过；独立 Java 语言语义诊断通过。主审与子 agent 均完成相关源码与调用链核验。 - **审查环境未重跑 FE UT、checkstyle、BE UT 或集群回归**（内部审查目录缺少构建前置的第三方依赖）。PR 描述中的 33 项 FE UT、3 项定向复跑和 checkstyle 通过，是作者提供的验证记录，不冒充本次执行。 - 三个新测试能区分本次 ABA 场景，但没有单独隔离「同一 info 仅时间戳改变」「扫描后延长 retention」两个条件，也未直接断言 erase journal/cleanup 调用为零；可后续补充。当前零副作用结论来自源码控制流核验，不表述成测试已经全面证明。 - 另见 [TRAE agent 的验证记录](https://github.com/apache/doris/pull/67750#issuecomment-5611033920)：报告 33 项 FE UT 和双 FE 滚动升级后的 6 个回归套件通过。这补充了外部集成证据；本次未独立检查其测试环境或原始 surefire/回归报告。 -- 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]
