morningman commented on code in PR #69749:
URL: https://github.com/apache/doris/pull/69749#discussion_r4401397501
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1680,6 +1684,34 @@ private void setupComputeGroup(ConnectContext ctx) {
}
}
+ private void applyComputeGroup(ConnectContext ctx) {
+ // A compute group declared on the MV pins every refresh, automatic or
manual, to that group.
+ // Only when the MV declares nothing does a manual REFRESH keep
borrowing the session's group,
+ // which is the behaviour every existing MV keeps.
+ String effective =
mtmv.getComputeGroup().orElse(taskContext.getComputeGroup());
+ if (!Strings.isNullOrEmpty(effective)) {
+ // A no-op outside cloud mode.
+ ctx.setCloudCluster(effective);
Review Comment:
This is the existing precedence between a statement hint and an object's
properties rather than something this PR introduces. `compute_group` is an
alias of the `cloud_cluster` session variable, and a `SET_VAR` hint in the MV's
own query is applied during analysis (`EliminateLogicalSelectHint` ->
`setVarOnceInSql`), after the task has prepared its context. On master the same
hint already overrides the group of an MV that declares none, and
`workload_group` behaves the same way: `MTMVPlanUtil.createMTMVContext` copies
the MV property into the session, and a `SET_VAR(workload_group=...)` in the
query overrides it while SHOW keeps reporting the property; the compute group
the task records is the declared one in the same way. The hint is part of the
MV definition its owner wrote, so the refresh following it is consistent with
every other session variable. Making the declaration win, or rejecting
conflicting hints at CREATE, would be a new rule for hints in MV SQL that
should cover `wo
rkload_group` too, which is outside this port. branch-4.x behaves the same.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -331,6 +332,10 @@ public void run() throws JobException {
}
mtmvSchemaChangeVersion = mtmv.getSchemaChangeVersion();
ConnectContext ctx = MTMVPlanUtil.createMTMVContext(mtmv,
MTMVPlanUtil.DISABLE_RULES_WHEN_RUN_MTMV_TASK);
+ // The planning done on this context (base table resolution, partition
calculation) must see
+ // the same compute group the refresh will execute in, otherwise the
workload group would be
+ // looked up in a different namespace.
+ applyComputeGroup(ctx);
Review Comment:
The window is real but very narrow, and its outcome is one failed refresh
with an explicit error rather than a wrong result: an ALTER that changes both
properties would have to commit between `createMTMVContext` reading the
workload group and `applyComputeGroup` reading the compute group, a few
statements apart on the refresh thread. That refresh then fails its workload
group lookup in the new group (`Can not find workload group ... in compute
group ...`), and the next one reads a consistent pair. branch-4.x has the same
two reads, and this port keeps them: reading both properties under one MV read
lock would close the window, but with no wrong result at stake we would rather
not diverge from 4.x here.
--
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]
