yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4090990571
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -510,9 +565,46 @@ && hasUnusableIvmStream()) {
mtmv.getName(), getTaskId());
return Lists.newArrayList(RefreshAttemptType.COMPLETE);
}
+ // Every partition either needs a rebuild or was never filled, and at
least one needs a rebuild:
+ // COMPLETE then does nothing the per-partition routing would not, in
one read of the MV.
+ if (!request.explicitPartitions &&
attempts.contains(RefreshAttemptType.IVM)
Review Comment:
Fixed, and confirmed as the substantive one of the four. The requirement is
durable and per partition, and only the requests that reach the incremental
attempt were reading it: `planPartitionRefresh` asked `isMTMVSync` -- the
snapshots -- and returned an empty plan, so a `PARTITIONS` request over the
failed-whole-MV-refresh state reported NOT_REFRESH and repaired nothing, while
a strict request that may not fall back got that same silent success instead of
the failure its own contract promises. The plan reads the requirement as well
now (`MTMV#getPartitionsNeedingRebuild`), both `PARTITIONS` forms rebuild the
partitions it names, and the incremental refresh after them reports nothing
left to rebuild -- the requirement cleared rather than deferred.
Of your two options I took the first (plan by the requirement) rather than
refuse or force: a partition refresh of one of those partitions is exactly the
rebuild the requirement asks for -- it is what the incremental attempt's own
rebuild phase calls -- so refusing would leave a job whose refresh method is
PARTITIONS unable to recover the MV at all, which is the worse end of the same
defect.
Covered end to end by a new nonConcurrent suite,
`test_ivm_partitions_after_failed_complete`: it produces the state by injecting
the failure into a real whole-MV refresh (a debug point on the IVM insert, so
the mark is journaled and the snapshots are never written), then runs both
`PARTITIONS` forms and pins the whole-MV scope, the row that was pending, and
the following incremental refresh reporting nothing to rebuild. Reverting just
this change makes it fail with the snapshot-only scope (`PARTIAL`), as the
positive control.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -510,9 +565,46 @@ && hasUnusableIvmStream()) {
mtmv.getName(), getTaskId());
return Lists.newArrayList(RefreshAttemptType.COMPLETE);
}
+ // Every partition either needs a rebuild or was never filled, and at
least one needs a rebuild:
+ // COMPLETE then does nothing the per-partition routing would not, in
one read of the MV.
+ if (!request.explicitPartitions &&
attempts.contains(RefreshAttemptType.IVM)
+ && shouldEscalateToComplete()) {
+ LOG.info("Every MV partition needs a rebuild or has no data yet,
mv={}, taskId={}. "
+ + "Continuing with COMPLETE refresh.", mtmv.getName(),
getTaskId());
+ return Lists.newArrayList(RefreshAttemptType.COMPLETE);
+ }
return attempts;
}
+ /**
+ * Notes that this refresh rebuilds partitions the request did not ask to
rebuild, which is what a
+ * strict INCREMENTAL request cannot tell from its result otherwise: it
reports the count, and a request
+ * that asked for a complete refresh reports nothing because rebuilding
everything is what it asked for.
+ */
+ private void recordRebuiltPartitions(RefreshRequest request, int
rebuiltPartitions) {
+ // Only an IVM MV has a baseline to rebuild: a plain MV's COMPLETE is
the only way it refreshes at
+ // all, so reporting it there would put a rebuild count on every
ordinary refresh.
+ if (!mtmv.isIvm() || request.refreshMode == RefreshMode.COMPLETE) {
+ return;
+ }
+ ivmRebuiltPartitions = rebuiltPartitions;
Review Comment:
Fixed. Where I agree, and where I read the consequence more narrowly.
Agreed, and this is what changed: the two columns were reassigned per phase,
so a refresh that rebuilt a partition and then fell back reported only what the
fallback covered, while the MV published the rebuilt partition's snapshots and
its epochs. They are task-wide accumulators now -- sets, because a later
attempt's scope can cover an earlier one's (a whole-MV attempt after a
per-partition one) and a partition counted twice would report more work than
the MV has partitions -- and `IvmRebuiltPartitions` is the largest count the
attempts reported rather than the last one's, for the same reason. That also
let the round-3 merge block go: the union is at the source now, so a phase
cannot narrow what an earlier one reported.
Where I did not follow the finding's conclusion: "SHOW loses work the task
actually committed" holds for the two columns, but I could not construct a case
where a *successful* task reports a wrong rebuilt count. The
overwrite-with-zero needs the whole-MV arm to be last and the task still to
succeed, and that arm returns: if the attempt throws, the task fails and
nothing is published, and its early `NOT_REFRESH` return is unreachable as long
as the arm sets its scope to the MV's partition names. So what I fixed there is
the fragility of "last writer wins" rather than a wrong value today. The record
keeps its shape in one more respect: both columns are still JSON arrays of
names -- a task written by an earlier FE reads into the sets and the sets write
the same JSON, which a test pins.
--
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]
