yujun777 commented on code in PR #69896:
URL: https://github.com/apache/doris/pull/69896#discussion_r4424600777
##########
be/src/storage/task/engine_publish_version_task.cpp:
##########
@@ -248,6 +250,18 @@ Status EnginePublishVersionTask::execute() {
tablet_id_to_num_delta_rows.insert(
{rowset_meta_ptr->tablet_id(),
rowset_meta_ptr->num_rows()});
}
+ // A load which writes a changelog keeps its changelog rowset on
the base tablet's
+ // transaction info instead of committing a changelog tablet of
its own, so its rows are
+ // reported separately below, once the publish tasks have
finished: a merge-on-write
+ // publication can still append changelog segments to that rowset.
The frontend cannot
+ // derive them from the rows it loaded into the base tablets -- an
update writes change
+ // rows without adding a base row, and a delete adds none to the
base but at least one
+ // to the changelog -- and it keeps the count to estimate how many
rows a read of the
+ // changelog returns.
Review Comment:
The retry path here has the same guarantee as the base index delta rows, and
I kept it that way rather than diverging:
- Within one `execute()` the retry loop is inside the function and the
collected changelog rowsets are declared before it, so a retry of the same
attempt keeps the rowset of a tablet that already published (the rowset object
stays valid, and its row count is the final one).
- Across *attempts* the report is discarded as a whole:
`PublishVersionDaemon.calculateTaskUpdateRows` skips a task that reports any
error tablet, for the base delta rows as well as for these. So a tablet that
published in a failed attempt contributes its delta with the next report, which
is the periodic tablet report and includes the changelog index. That is the
existing behaviour of the base index counter, and changing it means aggregating
per-tablet reports across attempts on the frontend, which I would rather do for
both counters in one change than only for the changelog one.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1525,6 +1535,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event)
{
+ "rows for all replica {}, tablets count {}",
olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
Review Comment:
Agreed, and it is recorded as a known limitation in the description: a cloud
commit reports rows per table from the meta-service `calc_table_stats`, which
sums only `base_tablet_ids`, so there is no per-index value to route. Until
that path carries the changelog rows, a cloud stream read keeps falling back to
the row count of the table, which is the behaviour before this PR — safe, just
loose. Feeding the cloud counter needs the meta-service side to sum the
attached changelog rowsets as well; I would keep it in a follow-up rather than
growing this one into the cloud txn path.
--
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]
