englefly commented on PR #68282: URL: https://github.com/apache/doris/pull/68282#issuecomment-5763964435
Review follow-up pushed in 643c00b3ce0. Three of the five findings were defects of the previous revision and are fixed with tests; the remaining two are explained below with the evidence I verified in the code, plus what I propose to do about them. Fixed: 1. **The replay reset was deleted by the outer replay wrapper** — `Env.replayTruncateTable()` removed the record right after `InternalCatalog.replayTruncateTable()` had reset it, so on a follower, and on any FE restart whose journal range covers the truncate, the record was dropped again. Fixed; verified on a local FE+BE that the record now survives a restart (`updated_rows=5` before, `updated_rows=5` after, `7` after two more loaded rows). 2. **The baseline advanced without refreshing the base index row count** — it is now advanced only when the job's `indexesRowCount` contains the base index id, so an MV-only or named-partition analysis no longer swallows the base index delta. New unit test. 3. **The reset and truncate journals were not atomic** — `resetTableStats()` no longer writes its own `OP_UPDATE_TABLE_STATS` entry; the reset is carried by the truncate entry and applied by `InternalCatalog.replayTruncateTable()` on every replay, for the DDL path and for followers/restart alike. Explained, and offered as a follow-up rather than silently dropped: 4. **Pre-truncate transaction deltas can be counted after the reset** — mechanism confirmed (`finishCheckPartitionVersion()` prunes the partition commit info but `updateCatalogAfterVisible()` forwards the unfiltered tablet delta map), and it is a pre-existing accounting gap that keeping the record makes observable. Impact is bounded to the planner fallback before the backends report. The fix belongs to the transaction/load accounting (filter by surviving partitions/tablets, or fence with a truncate generation in `TruncateTableInfo`) and needs a tablet existence check that must stay cheap on the load path. 5. **Stale asynchronous producers** — for the analyze case the previous revision applied the same stale snapshot by creating a new record, and the stale row count and the stale baseline cancel each other out (unit test `testDeltaRowCountOfSnapshotTakenBeforeReset` documents it). For `DropStatsTask` the window already exists for partition truncations and for analyzed tables; keeping the record widens it, which is a valid objection. Both want one epoch mechanism plus a generation-aware cleanup; I would rather land it as one coherent change than as a fence covering a single producer. Tests on this revision (local FE + BE cluster, single backend): `TableStatsMetaTest` 7/7, `test_row_count_after_truncate` passed, `test_drop_stats_and_truncate` passed, and the end to end case still reports `cardinality=3` after `TRUNCATE TABLE` + immediate load instead of `1`. -- 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]
