wenzhenghu commented on PR #67750: URL: https://github.com/apache/doris/pull/67750#issuecomment-5611031982
> 迁移自内部镜像 PR（HYDCP/hy-doris#103）的静态 review，行号已对齐本 PR。 ## 静态 Code Review：[fix](fe) Prevent stale recycle candidates from erasing new generations 纯静态 review（未运行 UT/回归测试，运行时验证见后续评论）。 ### 总体结论 ✅ **LGTM，建议合入。** 修复的是 #61366 "读锁收集候选 + 逐对象写锁擦除"模式残留的 ABA 竞态：候选收集后、写锁擦除前，对象被 recover 并以同 ID 重新进站时，旧候选会误擦新代并写入 erase journal。快照身份校验设计正确，三个过期路径（db/table/partition）统一覆盖，测试在精确窗口用 latch 确定性复现。 ### 修复正确性验证 **快照与校验机制**（`isSameGenerationAndExpired`，本 PR `CatalogRecycleBin.java` :318-323）： 1. `recycleInfoMap.get(id) == candidate.recycleInfo`（引用相等）：recover 会 remove、re-recycle 会 new 出新的 `RecycleXxxInfo` 对象——身份校验成立 ✅ 2. `idToRecycleTime.get(id) == candidate.recycleTime`：快照持有的是从 map 中取出的 Long，与 candidate 的 primitive `long` 比较为拆箱数值比较——任何 re-put 产生的新值（毫秒时间戳）与旧值不同即校验失败 ✅（注：此条最初按"装箱引用相等"理解，后续评论有更正，换代保护实际由第 1 条引用身份检查保证） 3. `isExpire(id, currentTimeMs)` 用 sweep 固定时间边界复查过期性 ✅ **短路顺序安全**：前两个检查任一失败即返回 false，`isExpire` 内的 `idToRecycleTime.get(id)` 拆箱不会 NPE（校验与拆箱同在写锁内，且失败路径先短路）✅ **Journal 正确性（关键）**：校验失败的候选零副作用——不 remove、不回调、不写 erase journal。这一点至关重要：若为陈旧候选写了 `logEraseTable`，follower 回放时会擦掉该 ID 当前映射的存活代，造成元数据损坏 ✅ **跳过后无饥饿**：被跳过的对象留在 map 中，下一轮 daemon 以新快照重新收集，新代按其新时间戳正常计算保留期 ✅ ### 未覆盖路径的防护核查（均确认安全，非阻塞） - **`eraseXxxWithSameName` 路径**（:389/:565/:710）：无 generation 校验，但有事实防护——每项擦除前 `isExpireMinLatency` **重新读取当前时间戳**，重回收的新代 latency ≈ 0 < 10min → 跳过；且该检查**不受** `catalog_trash_ignore_min_erase_latency` 影响（只有 `isExpire` 受其影响），生产配置下无法绕过。仅测试态（`FeConstants.runningUnitTest=true`）可暴露。建议（可选后续）：为一致性把 generation 校验同样应用于该路径，生产收益为零，纯统一性改进。 - **`eraseXxxInstantly`（drop force）路径**：recover-only 场景由 `get() == null` 防护；完整 ABA 需要"对象被 recover 且重新 drop"——但其父 db 正在被 force drop（不在 catalog），`RECOVER TABLE` 无处可挂载，实际不可达 ✅ - **`eraseAllTables`（db 擦除级联）**：使用已通过 generation 校验的 dbInfo 的 tableIds/tableNames 过滤 + 当前 `idToTable` 实时遍历——若 db 被 recover 过，db 候选校验先失败，级联不会执行 ✅ ### 关键检查点结论 | 检查点 | 结论 | |---|---| | 目标达成且最小 | ✅ 仅 2 个 FE 文件；修复与测试，无无关重构 | | 并发与锁 | ✅ 快照校验与全部删除副作用在同一写锁作用域内原子完成；无新锁、无新锁序边界 | | 生命周期 | ✅ 候选为单轮 daemon 局部对象，仅持有 recycle maps 已有的引用；测试的 executor/latch 在 finally 释放并恢复原锁 | | 兼容性 | ✅ 无持久化字段、image/journal 格式、RPC 变化 | | 平行路径 | ✅ db/table/partition 三条过期路径统一使用同一 helper；其余路径经核查有事实防护 | | 事务与持久化 | ✅ erase journal 仅在 generation 校验通过后写入 | | 性能 | ✅ 每个过期条目一个小对象 + O(1) 身份/时间戳检查，扫描与擦除复杂度不变 | ### 非阻塞建议 1. 为 `isSameGenerationAndExpired` 的时间戳比较语义补注释，明确换代保护由 `recycleInfoMap.get(id) == candidate.recycleInfo` 的引用身份检查保证，防未来维护者误解保护来源。 2. 陈旧候选被跳过时可考虑 debug 级日志或计数，便于观察生产上 ABA 实际发生频率（纯可观测性增强）。 3. （可选后续）`WithSameName` 路径统一应用 generation 校验。 -- 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]
