yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4089234425
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -259,7 +260,25 @@ private PartitionPlanningException(String message,
Throwable cause) {
// Written by the executing (Disruptor worker) thread via the
executeCommand consumer
// callback and read by the cancel (command) thread, so it must be
volatile.
private volatile StmtExecutor executor;
- private Map<String, MTMVRefreshPartitionSnapshot> partitionSnapshots;
+ // What this task has committed, per MV partition: the snapshot each
partition's rows were read at.
+ // One accumulator for the whole task rather than one per phase, because
that is what the MV publishes
+ // at the end of it -- a phase that started from empty would publish its
own work and drop the work of
+ // the phases before it, leaving partitions a preceding rebuild replaced
looking unsynced.
+ private Map<String, MTMVRefreshPartitionSnapshot> partitionSnapshots =
Maps.newConcurrentMap();
+ // The requirement each refreshed partition was read under, captured
before the base tables were read
+ // and recorded only once that batch's data committed (see
commitCapturedEpochs). In memory only: the
+ // journal carries the resulting states, and a replay applies those
instead of recomputing anything.
+ private transient Map<String, Long> ivmCapturedEpochs = Maps.newHashMap();
Review Comment:
Fixed: the map is concurrent, the getter hands out a detached copy, and
`MTMV#addTaskResult` reads it once so what it applies is what it journals. The
null case is answered too -- the field is transient, so a task read back from
the journal has none, and `addTaskResult` reads it on that path as well.
Two cases cover it: `MTMVTaskTest#testCapturedEpochsAreHandedOutAsACopy` and
`#testCapturedEpochsOfATaskReadBackFromTheJournalAreEmpty`.
What I did not change is what a cancelled task publishes: its epochs only
ever cover the batches that committed, which is the same rule `addTaskResult`
applies to a failed task's snapshots. Making the worker own terminal
publication would be a larger change to the task lifecycle than this one.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -343,6 +367,11 @@ public void run() throws JobException {
break;
case COMPLETE:
executeCompleteAttempt(refreshContext, ctx);
+ // Recorded here rather than where the escalation was
decided: the count is what the
+ // rebuild actually replaced, and a refresh that
failed before its first commit must
+ // not report the whole MV as rebuilt. The rebuild
records its own count for the
+ // partitions it replaced; this one is only reached
when it succeeded.
+ recordRebuiltPartitions(request,
mtmv.getPartitionNames().size());
Review Comment:
Fixed: the whole-MV attempt records its count in a `finally`, from the
groups that committed, the way the per-partition rebuild already did, and the
explicit-COMPLETE exemption stays. No unit test -- a multi-group COMPLETE
failing in a later group needs a harness the test class does not have; the
regression suites cover the success side.
--
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]
