github-actions[bot] commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4089552300
##########
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:
[P2] Keep dirty-rebuild reporting task-wide across later attempts. After p1
is rebuilt, a failed IVM attempt can reset the reporting lists and fall back:
PARTITIONS excludes p1 from its later scope and the success-only union at
787-790 never restores it, while a failing COMPLETE assigns only that later
attempt's count here (including zero) over the earlier commit. The task-wide
snapshot/epoch maps still publish p1, so SHOW loses work the task actually
committed. This is distinct from the fixed fallback data-retention thread;
accumulate unique rebuilt/scope/completed sets across every terminal path.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -342,7 +367,16 @@ public void run() throws JobException {
}
break;
case COMPLETE:
- executeCompleteAttempt(refreshContext, ctx);
+ try {
+ executeCompleteAttempt(refreshContext, ctx);
+ } finally {
+ // Counted from the groups that committed, like
the rebuild phase above: a
+ // whole-MV rebuild that failed in a later group
has still replaced the groups
+ // before it, and those keep the epochs and
snapshots they published (see
+ // MTMV#addTaskResult), so reporting none of them
would hide the partial work
+ // this count exists to expose.
+ recordRebuiltPartitions(request,
completedPartitions.size());
Review Comment:
[P2] Initialize this accumulator before entering COMPLETE.
completedPartitions is assigned only inside the incremental/partition
executors, but executeCompleteAttempt can return early for an MV with no
partitions or throw from reconcileIvmStreams before either one runs. This
finally then evaluates completedPartitions.size() first, turning the valid
empty COMPLETE into an NPE and masking the actionable reconciliation exception
(recordRebuiltPartitions's guard runs too late). Initialize it to an empty list
before the try, or count null-safely, and cover both pre-executor exits.
##########
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;
+ }
+
+ /**
+ * Whether every MV partition needs a rebuild, which is when COMPLETE does
nothing the per-partition
+ * routing would not.
+ *
+ * <p>A partition that holds data and does not need one makes this false:
COMPLETE would recompute it
+ * for nothing, which is the waste the per-partition routing exists to
avoid. A partition that was
+ * never refreshed does not count against it -- COMPLETE fills it, which
its routing branch would do as
+ * well -- and it needs no clause of its own: an aligned entry is {@code
{0, 1}}, so it is behind its
+ * requirement already. An MV with no partitions is not an escalation
either.
+ */
+ private boolean shouldEscalateToComplete() {
+ Map<String, MTMVPartitionState> states = mtmv.getPartitionStates();
Review Comment:
[P2] Avoid a second full state snapshot on every IVM task.
getPartitionStates deep-copies the map and every MTMVPartitionState while
holding mvRwLock; in the normal not-all-dirty case this copy is discarded, then
executeIvmAttempt immediately makes the same full copy again for routing. At
the existing 160k-partition scale that is about 320k DTO allocations plus two
maps even for a no-op refresh. This is separate from the fixed ADD_TASK payload
issue. Use an allocation-free lock-scoped all-dirty predicate here and retain
only the one detached routing snapshot.
##########
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:
[P2] Do not hide failed-COMPLETE epochs from PARTITIONS chains.
executeCompleteAttempt raises every partition requirement before stream
reconciliation but keeps the old snapshots; if reconciliation or the first
rebuild batch fails, a later PARTITIONS or PARTITIONS FALLBACK request has no
IVM attempt, skips this check, and its snapshot planner can return
NOT_REFRESH/SUCCESS before the fallback COMPLETE. The MV still has dirty epochs
and the baseline recovery the previous task made durable. Include dirty live
partitions in PARTITIONS planning (or otherwise force/reject the recovery), and
cover failed COMPLETE followed by both PARTITIONS forms.
--
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]
