Repository navigation
[fix](binlog) Wrong judgment of anyEnable for add binlog (#48919) - #77
Conversation
Related PR: apache#40680 View doesn't have the binlog config cache, so the binlog is not enabled by default when a view is created. (cherry picked from commit bf50dd5)
wenzhenghu
left a comment
wenzhenghu
left a comment
There was a problem hiding this comment.
代码逻辑本身看起来没有问题，这个修改把单表场景下的 anyEnable 判定从覆盖 dbBinlogEnable 改成了 OR 语义，和 DBBinlog 里的后续处理是一致的。
不过这里建议补一个 FE UT 覆盖回归场景：db binlog = true、table binlog = false、且 tableIds.size() == 1。当前仓库里的 BinlogManagerTest 主要反射调用的是 addBinlog(TBinlog, Object)，并没有覆盖这次修改的 addBinlog(long dbId, List<Long> tableIds, ...) 判定路径；同时现有 mock 里 isEnableTable() 也是固定返回 true，旧问题不会被测出来。
我本地执行了 ./run-fe-ut.sh --run org.apache.doris.binlog.BinlogManagerTest，结果通过，但它还不能证明这次修复点。
### What problem does this PR solve? Issue Number: N/A Related PR: apache#48919 Problem Summary: Add a regression test for the single-table binlog gate when database-level binlog is enabled and table-level binlog is disabled. The test invokes the policy-aware addBinlog overload and verifies the event is retained. ### Release note None ### Check List (For Author) - Test: No need to test (worktree lacks thirdparty/installed/bin/protoc; git diff --check passed) - Behavior changed: No - Does this need documentation: No
wenzhenghu
left a comment
wenzhenghu
left a comment
There was a problem hiding this comment.
新增 UT 的方向是对的，已经命中了这次修复的 addBinlog(long dbId, List<Long> tableIds, ...) 路径。
不过这里还有一个断言问题：如果 enableDbBinlog = true，即使 enableTableBinlog = false，DBBinlog.getTableBinlog() 仍会因为 dbBinlogEnable || isEnableTable(...) 成立而创建 TableBinlog，并把 table dummy binlog 加到 tableDummyBinlogs。而 DBBinlog.getAllBinlogs() 返回的是 tableDummyBinlogs + allBinlogs。
因此这个场景下 binlogs.size() 预期应当是 3，而不是 2：
- table dummy binlog
- db dummy binlog
- 真实的 UPSERT binlog
所以当前这条测试在现有实现语义下应当会失败，建议把数量断言改掉，或者直接改成断言最后一条 binlog 的 commitSeq/type，避免把 dummy 计数细节写死。
### What problem does this PR solve? Issue Number: N/A Related PR: apache#48919 Problem Summary: Refine the DB-level binlog regression test to assert the emitted UPSERT event instead of an implementation-specific number of dummy binlogs. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.binlog.BinlogManagerTest - Behavior changed: No - Does this need documentation: No
xuchenhao
left a comment
xuchenhao
left a comment
There was a problem hiding this comment.
已审查 head 3d8d75c0470，没有发现阻塞问题，LGTM。
anyEnable现在保留 DB 级结果，并与表级结果做 OR；db binlog = true、单表缓存返回 false 时不会再错误丢弃事件。该生产代码改动与上游 apache/doris#48919 一致。- 新增 UT 命中了
addBinlog(long dbId, List<Long> tableIds, ...)的回归路径，覆盖db=true / table=false / tableIds.size()==1，并通过type + commitSeq验证真实 UPSERT，避免依赖 dummy binlog 数量。
本地验证：
mvn -B -ntp -pl fe-core -am test -Dcheckstyle.skip=true -DfailIfNoTests=false -Dtest=org.apache.doris.binlog.BinlogManagerTest
结果：Tests run: 5, Failures: 0, Errors: 0, Skipped: 0，fe-common 和 fe-core 均构建成功；git diff --check 也通过。
补充：当前 Clang Formatter 和 License Check 的失败都发生在 actions/checkout@v3 阶段；pull_request_target 工作流拒绝检出跨仓 PR 代码，实际 formatter/license 检查尚未运行，因此这两项失败与本 PR diff 无关，需要仓库工作流侧处理或由维护者重跑。Check large file 已通过。
静态 Code Review：fix Wrong judgment of anyEnable for add binlog (apache#48919)
总体结论✅ LGTM，建议合入。 修复分析
行为影响枚举核对无误：仅"db=true / table=false / 单表"组合从错误丢弃变为正确记录，其余组合（db=false、多表任一 enable 等）行为不变。改动 4 行，最小且正确。 关键检查点
测试
|
What problem does this PR solve?
Issue Number: N/A
Related PR: apache#48919
Problem Summary:
For a binlog event with exactly one table ID, 3.1 assigns
anyEnable = tableBinlogEnableand overwrites the prior database-level result. Thus a database withbinlog.enable=truecan incorrectly skip the event when the table cache returns false, includingswap=false REPLACE TABLEafter a cold cache.This is a clean cherry-pick of upstream apache#48919. It retains the database result and combines table results with OR.
Release note
Fixes eligible single-table binlog events being skipped when database-level binlog is enabled.
Check List (For Author)
git diff --checkpassed. The worktree lacksthirdparty/installed/bin/protoc, so FE tests could not run.