yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4090988743
##########
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:
Fixed, and the allocation is gone rather than reduced: the verdict is
answered in place, under the MV read lock, by the MV that owns the states
(`MTMV#allPartitionsNeedRebuild`), so the ordinary not-all-dirty case allocates
nothing at all. `MTMVTask#shouldEscalateToComplete` is deleted and the call
site asks the MV directly. The detached snapshot `executeIvmAttempt` takes for
routing is now the only full copy, as you suggested.
##########
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:
Fixed, and taken further than "initialise it before the try": the count is
read from the same accumulator the task reports its progress with, null-safely,
so there is one source for both call sites rather than two shapes of the same
number, and an attempt that committed nothing reads as zero instead of
throwing. Both pre-executor exits you named are covered by unit tests now --
the empty whole-MV refresh, and the one the stream reconciliation throws out of
-- and removing the null-safe read makes the first of them fail with exactly
the NPE you described.
--
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]
