github-actions[bot] commented on code in PR #69028:
URL: https://github.com/apache/doris/pull/69028#discussion_r4345864730
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1298,12 +1344,12 @@ public void updateUpdatedRows(Map<Long, Long>
partitionToUpdateRows, long dbId,
}
// Invoke this for cloud version load.
- public void updateUpdatedRows(Map<Long, Long> updatedRows) {
+ public void updateUpdatedRows(Map<Long, Long> updatedRows, long dbId) {
try {
if (!Env.getCurrentEnv().isMaster() || Env.isCheckpointThread()) {
return;
}
- UpdateRowsEvent updateRowsEvent = new UpdateRowsEvent(updatedRows);
+ UpdateRowsEvent updateRowsEvent = new UpdateRowsEvent(dbId,
updatedRows);
Review Comment:
[P2] Serialize cloud row updates with statistics deletion
The new DROP STATS table lock does not protect the cloud path:
`reportCommitTxnResult() -> afterCommitTxnResp() -> updateUpdatedRows()` takes
no target-table lock. After a cloud report creates a record containing N rows,
pause between `replayUpdateRowsRecord()` and `logUpdateRowsRecord()`, then
complete whole-table DROP STATS. The master removes the record and journals
DELETE; resuming the report journals UPDATE_ROWS. Replay therefore executes
DELETE -> UPDATE_ROWS and recreates N, while the master has no record. Before
tablet reporting, `Project(*) -> OlapScan(t)` estimates 1 on the master versus
N after failover. This is distinct from the existing non-cloud VISIBLE/DROP
STATS thread: it uses the separate cloud journal and the opposite replay order.
Serialize cloud mutation through journal completion with the deletion protocol,
and cover this interleaving with barriers.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1355,7 +1401,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event)
{
InternalCatalog catalog = Env.getCurrentInternalCatalog();
if (event.getRecords() != null) {
for (Entry<Long, Long> record : event.getRecords().entrySet()) {
- TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
+ TableStatsMeta statsStatus = findOrCreateTableStats(catalog,
event.getDbId(), record.getKey());
Review Comment:
[P2] Exclude pre-truncate cloud reports from the recreated record
Cloud stream loads queue `send_stats_to_fe_async()` and can return success
before the FE receives their statistics. Load 100 rows, delay that report,
TRUNCATE the entire table, then deliver the old response and load 3 new rows.
TRUNCATE keeps the table ID, so this branch recreates a zero-baseline record
and accumulates the removed 100 rows; the next load raises it to 103.
`afterCommitTxnResp()` applies these counters before `updateVersion()` skips
the now-missing old partition. While the replacement tablets are unreported,
`Project(*) -> OlapScan(t)` consequently estimates 103 for 3 rows. Previously
the delayed report found no record and was ignored. This is separate from the
known partial-truncate/drop-partition counter issue. Validate the report's
partition generation before recreating/accumulating the record, and add a
delayed-report/full-truncate/reload case.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1366,52 +1412,52 @@ public void replayUpdateRowsRecord(UpdateRowsEvent
event) {
// Record : TableId -> (TabletId -> update rows)
if (event.getTabletRecords() != null) {
for (Entry<Long, Map<Long, Long>> record :
event.getTabletRecords().entrySet()) {
- TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
- if (statsStatus != null) {
- Optional<Database> dbOption =
catalog.getDb(event.getDbId());
- if (!dbOption.isPresent()) {
- LOG.warn("Database {} does not exist.",
event.getDbId());
- continue;
- }
- Database database = dbOption.get();
- Optional<Table> tableOption =
database.getTable(record.getKey());
- if (!tableOption.isPresent()) {
- LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
- continue;
- }
- Table table = tableOption.get();
- if (!(table instanceof OlapTable)) {
- continue;
- }
- OlapTable olapTable = (OlapTable) table;
- short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
- Map<Long, Long> tabletRows = record.getValue();
- if (tabletRows == null || tabletRows.isEmpty()) {
- LOG.info("Tablet row count map is empty");
- continue;
- }
- long rowsForAllReplica = 0;
- for (Entry<Long, Long> entry : tabletRows.entrySet()) {
- rowsForAllReplica += entry.getValue();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Table id {}, tablet id {}, row count
{}",
- record.getKey(), entry.getKey(),
entry.getValue());
- }
- }
- long tableUpdateRows = rowsForAllReplica / replicaNum;
- LOG.info("Update rows for table {} is {}, replicaNum is
{}, "
- + "rows for all replica {}, tablets count {}",
- olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
- statsStatus.updatedRows.addAndGet(tableUpdateRows);
- if (StatisticsUtil.enablePartitionAnalyze()) {
- updatePartitionRows(olapTable, tabletRows,
statsStatus, replicaNum);
+ Optional<Database> dbOption = catalog.getDb(event.getDbId());
+ if (!dbOption.isPresent()) {
+ LOG.warn("Database {} does not exist.", event.getDbId());
+ continue;
+ }
+ Database database = dbOption.get();
+ Optional<Table> tableOption =
database.getTable(record.getKey());
+ if (!tableOption.isPresent()) {
+ LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
+ continue;
+ }
+ Table table = tableOption.get();
+ if (!(table instanceof OlapTable)) {
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ TableStatsMeta statsStatus = findOrCreateTableStats(olapTable);
Review Comment:
[P2] Order schema-completion removal with load record recreation
`SchemaChangeJobV2.runRunningJob()` journals FINISHED under the table write
lock, unlocks at line 732, and removes table statistics only afterwards at line
738. A ready load can become VISIBLE in that gap and update the old record,
which the schema worker then removes on the master. Replay applies FINISHED and
removes statistics in `EditLog`, then replays VISIBLE; this new call recreates
a record containing the load's N rows. The master thus has no record while
replay/checkpoint has N. With the new index still unreported, `Project(*) ->
OlapScan(t)` estimates 1 versus N after failover. The previous get-only path
left both records absent. This is a distinct schema-completion caller,
untouched by the DROP STATS locking fix. Remove statistics within the schema
FINISHED/table-lock boundary and add a deterministic schema-completion/load
interleaving test.
--
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]
