englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4089380127
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -1058,7 +1058,7 @@ public boolean unprotectDropTable(Database db, Table
table, boolean isForceDrop,
if (table instanceof BaseTableStream) {
Env.getCurrentEnv().getTableStreamManager().removeTableStream((BaseTableStream)
table);
}
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(table.getId());
+
Env.getCurrentEnv().getAnalysisManager().removeTableStatsAndLog(table.getId());
Review Comment:
Fixed at `f1359ea6196`: this call site uses the plain
`removeTableStats(table.getId())` again.
You are right on both counts, and the change was mine to revert —
`unprotectDropTable` is the shared body of the live DROP and of the replay
(`replayDropTable`, `replayDropDb` per table, replay-time MTMV/IVM stream
cleanup all pass `isReplay`), so appending `OP_DELETE_TABLE_STATS` from it
writes to the edit log while it is being replayed, which is fatal on a BDB
replica, and on the live path it made the removal durable before
`logDropTable`/`logDropDb`. The enclosing DROP owns the transition: its entry
replays this same code on every frontend, so the record is removed once per
frontend without a separate entry, exactly as before this PR.
Test, as you asked, through the real dispatcher with an edit log that
rejects writes:
`StatisticsRecordReplayTest#testDropTableReplayDoesNotJournalTheStatsDeletion`
seeds a statistics record, swaps in `Mockito.spy(env.getEditLog())` whose
`logDeleteTableStats` throws `AssertionError("a replay must not write
OP_DELETE_TABLE_STATS")`, calls `Env.getCurrentEnv().replayDropTable(db,
tableId, true, 0L)` on a real frontend (`TestWithFeService`), and asserts the
record is gone afterwards. I ran the same test against a version with
`removeTableStatsAndLog` put back: it fails with that exact assertion error, so
it is not a vacuous pass.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -812,7 +812,7 @@ private void replaceTableInternal(Database db, OlapTable
origTable, OlapTable ne
} else {
Env.getCurrentRecycleBin().recycleTable(db.getId(), origTable,
isReplay, isForce, 0);
}
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(origTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().removeTableStatsAndLog(origTable.getId());
Review Comment:
Fixed at `f1359ea6196`: this branch uses the plain
`removeTableStats(origTable.getId())` again.
Confirmed: `replayReplaceTable()` reaches `replaceTableInternal(...,
isReplay=true, ...)`, so the non-swap branch appended `OP_DELETE_TABLE_STATS`
while a follower was replaying `OP_REPLACE_TABLE` (fatal on a BDB replica), and
on the live path it made the removal durable before `logReplaceTable`, leaving
the replaced table without its statistics if the outer entry never landed.
`OP_REPLACE_TABLE` owns the transition: its replay runs this same branch, so
the record is removed on every frontend without a separate entry.
Test through the real dispatcher with an edit log that rejects writes:
`StatisticsRecordReplayTest#testReplaceTableReplayDoesNotJournalTheStatsDeletion`
creates the replaced and the replacing table, seeds a statistics record for
the replaced one, swaps in a spied edit log whose `logDeleteTableStats` throws,
calls `Env.getCurrentEnv().getAlterInstance().replayReplaceTable(new
ReplaceTableOperationLog(dbId, origId, "t_replace_orig", newId,
"t_replace_new", false, true))`, and asserts the record is gone. Verified as a
negative control as well: with the journaling removal put back, this test fails
with "a replay must not write OP_DELETE_TABLE_STATS".
The journal pairing you reviewed earlier is untouched where it belongs — the
transitions which journal on their own (whole-table DROP STATS from the cleaner
and the auto collector, rename, schema change) still write their entry together
with the mutation under `idToTblStats`.
--
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]
