morningman commented on code in PR #69856:
URL: https://github.com/apache/doris/pull/69856#discussion_r4419693837
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1349,6 +1423,8 @@ private void
executePartitionBasedRefresh(MTMVRefreshContext context, RefreshMod
mtmv.getName(), getTaskId(), e);
throw new JobException(e.getMessage(), e);
}
+ recordCommitted(refreshMode == RefreshMode.COMPLETE
+ ? IvmRefreshMethod.COMPLETE :
IvmRefreshMethod.PARTITION_REBUILD);
Review Comment:
Thanks. The race is real, but its reach is narrower than described, and the
report already follows the cut master applies to a canceled task.
- Only a cancel publishes from another thread. `onSuccess` and `onFail` run
`after()` on the worker once `run()` has returned, so a task that succeeds or
fails is quiescent when it is reported.
- A canceled task does not touch the summary: `IvmRefreshSummary.update`
returns on CANCELED, and `recordIvmRefreshMetrics` counts no fallback and no
duration for it. So the summary cannot end up saying `NONE`, and no fallback is
missed. What can come out short is the canceled task's own `IvmRefreshMethod`,
and the `method` label its
`doris_fe_ivm_refresh_tasks_total{result="canceled"}` count goes to.
- Only a batch whose transaction was already committing when the cancel
landed can be missed. A statement that has not started is rejected by
`registerExecutor`, a running one is aborted by the coordinator cancel, and
`refreshPartitions` checks CANCELED after its statement and throws before
`recordCommitted`. In practice the window is the commit of the delta.
- That is the cut master already applies to what a canceled task publishes.
`after()` runs on the cancel thread without waiting for the worker, and
publishes copies of the partition snapshots and the epochs, so a batch that
commits afterwards is not part of the result (#68390, see the comment in
`after()`). The method is decided at the same point, so it agrees with the
snapshots the task publishes. Counting a late batch in the method while its
snapshots stay out would make the record contradict itself.
- Waiting for the worker to stop is a change to the cancellation of the job
framework. `cancel(false)` comes from DROP MATERIALIZED VIEW and from an ALTER
that rebuilds the job (`JobManager.dropJobInternal` sets STOPPED), and it
deliberately does not block on a refresh in flight; `cancel(true)` only waits
for the statement (`waitNotRunning`), not for what the worker records after it.
That is outside the scope of this PR. The other half of the suggestion, one
terminal snapshot for the report, the metrics and the journal, is what
`after()` does now: the method, the failure reason, the counters and the
journaled record are all decided there, once.
So I'm keeping it as it is.
--
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]
