sundapeng commented on PR #8900: URL: https://github.com/apache/paimon/pull/8900#issuecomment-5114473293
## 🔍 DLF-Review 自动代码审查 (GAN) **模式**: FULL | **Score**: 87/100 **严重度分布**: major: 1, minor: 1, suggestion: 3 **GAN Stats**: 18 原始 → 8 去重 → Disc(4✓ 1? 3✗) → Arb(+1) → 5 最终 ### 代码问题 (4) #### 🟠 major 1. **[tests]** `RenamingTwoPhaseOutputStreamTest.java:93` — `testCleanLeavesTheStagingDirectoryForItsOwners` 检测不到它声称要保护的 staging 目录被删除：唯一有区分度的断言 `listStatus(stagingDir).isEmpty()` 对不存在的路径同样返回空数组，目录被整个删掉时测试仍绿。变异验证：回退为 `deleteDirectoryQuietly(parent)` 或改为 `deleteQuietly(parent)`（非递归删空共享目录，正是本 PR 描述的 race）本测试都不红，且后者整个 suite 无测试能杀死。修复：clean() 后加 `assertThat(fileIO.exists(stagingDir)).isTrue()`。（已发行内评论） #### 🟡 minor 2. **[migration]** `RenamingTwoPhaseOutputStream.java:139` — clean() 改为只删自身 tempPath 后，崩溃 writer（Flink failover/OOM，未走到 discard/clean）遗留的 `.tmp.UUID` 文件失去了唯一的 GC 路径：overwrite 的 `deletePreviousDataFile` 和读路径都按 `isDataFileName` 过滤掉 '.'/'_' 前缀不会删，`OrphanFilesClean` 只接受 FileStoreTable。结果 `_temporary` 无限膨胀，递归 listFiles 的 scan/overwrite 延迟随时间线性劣化。建议增加带时效阈值的 staging 清理或文档化外部 lifecycle 规则。（已发行内评论） #### 🟢 suggestion 3. **[architecture]** `TwoPhaseOutputStream.java:57`（不在本 PR diff 内，无法行内评论）— 本次修复确立的关键不变量——"clean() 只能删除本 committer 自己 stage 的内容，共享 staging 目录不属于它"——只以实现注释形式留在 `RenamingTwoPhaseOutputStream.TempFileCommitter` 里。接口 `Committer.clean()` 至今没有一行 javadoc（commit()/discard() 都有），且 discard() 的 javadoc 写着 "cleaning up any temporary files"，与 clean() 的职责边界在接口层面无法区分。两个实现的 clean() 语义已分叉（BaseMultiPartUploadCommitter 是 no-op，Renaming 删自己 staged 文件），该接口标注 @Public、是对外扩展点——下一个实现者从接口上看不到任何约束，可以原样重新引入本次修掉的共享目录误删 bug。**建议**：把契约上提到接口 javadoc（clean() 在 commit 成功后调用、只允许移除本 committer 自己 stage 的资源、禁止触碰共享目� ��），并一句话区分 clean() 与 discard() 的调用时机。 4. **[architecture]** `FormatTableCommit.java:200`（不在本 PR diff 内，无法行内评论）— 改动后 clean() 在唯一生产调用路径上退化为纯冗余 RPC：全仓 main 代码中 `committer.clean()` 仅 FormatTableCommit.commit() 一处，且只在所有 commit() 成功后执行，而 TempFileCommitter.commit()（L114-119）已 rename 走 tempPath 并做了 deleteQuietly 兜底。clean() 唯一有实际意义的分支——"未 commit 时删掉自己 staged 的文件"（testCleanRemovesTheFileItStagedWhenThereWasNoCommit 覆盖的场景）——在生产上不可达：失败路径走 catch -> abort() -> discard()，从不调 clean()。成功路径上 clean() 变成每个已提交文件对对象存储多发 1-2 次无效 RPC。与上一条指向同一根因：commit/discard/clean 生命周期职责边界不清。**建议**二选一收敛：要么明确 clean() 为 commit 成功后的兜底（接口注明可为 no-op，Renaming 实现与 commit() 内兜底删除二 取其一）；要么让失败/放弃路径也走 clean()，使该分支真正可达。@Public 兼容前提下以文档化为主。 ### 风格 / 文档 / 组织性建议 (advisory, 1) - **[naming]** `RenamingTwoPhaseOutputStreamTest.java:75` — 两个新测试名近义（Keeps vs Leaves），区分场景的关键变量（目录非空 vs 空）未体现在名字里，读者必须读方法体才能发现区别。建议把场景编码进名字，如 `testCleanKeepsStagingDirContainingOtherWritersFiles` 与 `testCleanKeepsEmptyStagingDir`（或 `...EvenWhenEmpty`）。不计入评分，宜独立 commit。 ### 👍 亮点 - 修复方式精准克制：clean() 从递归删除共享 _temporary 目录收敛为只删自己 staged 的 tempPath，用最小 diff 消除了并发写入者被误删的 race，且与 maintainer 在 known-findings 中要求的修复方向一致 - 新增测试覆盖了对抗性并发场景：testCleanKeepsTheSharedStagingDirectory 显式构造 MapReduce 风格的并发 pending attempt 文件，验证 clean() 不再殃及其他 writer，这类测试能直接杀死回退到递归删除的变异 - clean() 处的新注释把所有权语义讲清楚了（'seeing it empty does not mean it is unused'），准确记录了为什么不能删空目录——刚创建目录尚未 stage 文件的 writer 会被破坏——为后续维护者防住了最可能的回归 --- _Qoder DLF-Review (GAN)_ -- 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]
