yujun777 commented on code in PR #69772:
URL: https://github.com/apache/doris/pull/69772#discussion_r4400955292
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -828,6 +829,17 @@ private AttemptResultType
executeIvmAttempt(MTMVRefreshContext refreshContext,
this.refreshMode = generateRefreshMode(toRebuild);
try {
executePartitionBasedRefresh(refreshContext,
RefreshMode.PARTITIONS, ctx, toRebuild);
+ } catch (JobException e) {
+ // The rebuild reads the tables the MV does not partition by
through their streams at the
+ // offsets, so an offset changelog.ttl_seconds has expired
stops it the way it stops the delta
+ // below -- and it is the same failure of this attempt,
answered the same way.
+ String detail = Util.getRootCauseMessage(e);
+ if (IvmFailureClassifier.classifyExecutionFailure(detail)
Review Comment:
This check can state the invariant instead of enumerating the reason, and
pass the reason through:
```java
Optional<IvmFailureReason> reason =
IvmFailureClassifier.classifyExecutionFailure(detail)
.filter(IvmFailureReason::requiresCompleteRefresh);
if (reason.isEmpty()) {
throw e;
}
return handleIvmFallbackResult(IvmIncrRefreshResult.fallback(reason.get(),
detail), request);
```
`requiresCompleteRefresh()` is exactly the set whose only repair is
COMPLETE, which is what makes it valid to turn a rebuild failure into a
fallback from inside the rebuild: there is no meaningful PARTITIONS fallback
here (the rebuild already is the partition refresh) and the delta retry
machinery (`MV_PARTITION_NOT_FOUND` <-> sync + retry) does not exist on this
path. Behaviour is unchanged today, since the classifier can only produce
`CHANGELOG_BROKEN` here; the difference is that the code says why the
conversion is safe rather than which reason happened to be reachable, and the
synthesized result carries its real reason instead of a hard-coded one.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1184,11 +1196,12 @@ private AttemptResultType
handleIvmFallbackResult(IvmIncrRefreshResult ivmResult
+ ", detail=" + ivmResult.getDetailMessage());
}
if (ivmResult.getFailureReason() == IvmFailureReason.CHANGELOG_BROKEN)
{
Review Comment:
This branch is redundant now: `requiresCompleteRefresh()` already returns
true for `CHANGELOG_BROKEN` (`IvmFailureReason.java:41-47`), and the branch
below returns the same `FALLBACK_TO_COMPLETE` while its log already prints
`reason={}` and `detail={}`. The only difference is the log wording, so I would
drop this branch entirely and keep the single `requiresCompleteRefresh()`
branch. The explanation of why no partition attempt is tried before COMPLETE (a
partition refresh still reads the streams at their offsets) still reads well as
a comment on that generic branch, or next to the classifier entry that produces
the reason. If the catch above is also filtered on `requiresCompleteRefresh()`
(see the other comment), nothing remains here that is specific to
`CHANGELOG_BROKEN`.
--
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]
