This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 942734e94dc [fix](workload) enforce explicit compute group form for
workload DDLs (#63505)
942734e94dc is described below
commit 942734e94dc5ecd297cabc90ade035b88343f8a1
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Mon May 25 00:10:22 2026 -0700
[fix](workload) enforce explicit compute group form for workload DDLs
(#63505)
Cloud mode and non-cloud mode used to share fuzzy fallback behaviour for
the
compute-group qualifier on workload DDLs. The fallback misbehaved when
the user
omitted the qualifier in cloud mode:
CREATE WORKLOAD POLICY ... 'workload_group'='superset'
-> "Unable to find the compute group: <default_compute_group>"
DROP WORKLOAD GROUP superset
-> "Can not find workload group superset in compute group ."
The literal Tag.VALUE_DEFAULT_COMPUTE_GROUP_NAME was being passed
downstream as
if it were a real cluster name.
Enforce a strict contract instead of papering over with a
session-derived
fallback:
- Cloud mode : require '<compute_group>.<workload_group>' for workload
policy 'workload_group' property, and require FOR/FROM
clause on CREATE / ALTER / DROP WORKLOAD GROUP.
- Non-cloud mode: forbid the qualifier on all four; use
Tag.VALUE_DEFAULT_TAG
internally.
Both invalid inputs now surface clear UserException messages that tell
the user
how to spell the statement correctly.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
---
.../plans/commands/AlterWorkloadGroupCommand.java | 17 +-
.../plans/commands/CreateWorkloadGroupCommand.java | 30 +--
.../plans/commands/DropWorkloadGroupCommand.java | 13 +-
.../WorkloadSchedPolicyMgr.java | 55 +++++-
.../WorkloadSchedPolicyMgrTest.java | 206 +++++++++++++++++++++
5 files changed, 292 insertions(+), 29 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterWorkloadGroupCommand.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterWorkloadGroupCommand.java
index 730d74aa7da..4bd4c21c601 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterWorkloadGroupCommand.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterWorkloadGroupCommand.java
@@ -82,19 +82,24 @@ public class AlterWorkloadGroupCommand extends AlterCommand
{
throw new AnalysisException(WorkloadGroup.COMPUTE_GROUP + " can
not be set in property.");
}
- ComputeGroup cg = null;
+ ComputeGroup cg;
if (Config.isCloudMode()) {
if (StringUtils.isEmpty(computeGroup)) {
- computeGroup = Tag.VALUE_DEFAULT_COMPUTE_GROUP_NAME;
+ throw new UserException("Must specify compute group via 'FOR
<compute_group>' "
+ + "in cloud mode.");
}
- String cgName = computeGroup;
- cg =
Env.getCurrentEnv().getComputeGroupMgr().getComputeGroupByName(cgName);
+ cg =
Env.getCurrentEnv().getComputeGroupMgr().getComputeGroupByName(computeGroup);
if (cg == null) {
- throw new UserException("Can not find compute group:" +
cgName);
+ throw new UserException("Can not find compute group:" +
computeGroup);
}
} else {
+ // In non-cloud mode, 'FOR <compute_group>' is also supported
syntactically, but
+ // the value here actually refers to a resource group (Tag) —
there are no real
+ // compute groups in non-cloud mode. The grammar is shared with
cloud mode purely
+ // for consistency. When the clause is omitted, fall back to the
default resource
+ // group Tag.VALUE_DEFAULT_TAG.
if (StringUtils.isEmpty(computeGroup)) {
- computeGroup = Tag.DEFAULT_BACKEND_TAG.value;
+ computeGroup = Tag.VALUE_DEFAULT_TAG;
}
cg =
Env.getCurrentEnv().getComputeGroupMgr().getComputeGroupByName(computeGroup);
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateWorkloadGroupCommand.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateWorkloadGroupCommand.java
index 1668bccf992..ec029929716 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateWorkloadGroupCommand.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateWorkloadGroupCommand.java
@@ -96,24 +96,32 @@ public class CreateWorkloadGroupCommand extends Command
implements ForwardWithSy
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
validate(ctx);
- if (StringUtils.isEmpty(computeGroup)) {
- computeGroup = Config.isCloudMode() ?
Tag.VALUE_DEFAULT_COMPUTE_GROUP_NAME
- : Tag.DEFAULT_BACKEND_TAG.value;
- } else if (Config.isNotCloudMode()) {
- try {
- FeNameFormat.checkCommonName("Workload group's compute group",
computeGroup);
- } catch (AnalysisException e) {
- throw new DdlException("Compute group's format is illegal: " +
computeGroup);
- }
- }
-
if (Config.isCloudMode()) {
+ if (StringUtils.isEmpty(computeGroup)) {
+ throw new UserException("Must specify compute group via 'FOR
<compute_group>' "
+ + "in cloud mode.");
+ }
String originStr = computeGroup;
computeGroup = ((CloudSystemInfoService)
Env.getCurrentEnv().getClusterInfo()).getCloudClusterIdByName(
computeGroup);
if (StringUtils.isEmpty(computeGroup)) {
throw new UserException("Can not find compute group " +
originStr + ".");
}
+ } else {
+ // In non-cloud mode, 'FOR <compute_group>' is also supported
syntactically, but
+ // the value here actually refers to a resource group (Tag) —
there are no real
+ // compute groups in non-cloud mode. The grammar is shared with
cloud mode purely
+ // for consistency. When the clause is omitted, fall back to the
default resource
+ // group Tag.VALUE_DEFAULT_TAG.
+ if (StringUtils.isEmpty(computeGroup)) {
+ computeGroup = Tag.VALUE_DEFAULT_TAG;
+ } else {
+ try {
+ FeNameFormat.checkCommonName("Workload group's compute
group", computeGroup);
+ } catch (AnalysisException e) {
+ throw new DdlException("Compute group's format is illegal:
" + computeGroup);
+ }
+ }
}
// Create workload group
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropWorkloadGroupCommand.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropWorkloadGroupCommand.java
index a50c09cc461..28aaf0448c6 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropWorkloadGroupCommand.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropWorkloadGroupCommand.java
@@ -60,10 +60,11 @@ public class DropWorkloadGroupCommand extends DropCommand {
}
if (Config.isCloudMode()) {
- String originCgStr = computeGroup;
if (StringUtils.isEmpty(computeGroup)) {
- computeGroup = Tag.VALUE_DEFAULT_COMPUTE_GROUP_NAME;
+ throw new UserException("Must specify compute group via 'FOR
<compute_group>' "
+ + "in cloud mode.");
}
+ String originCgStr = computeGroup;
String clusterId = ((CloudSystemInfoService)
Env.getCurrentEnv().getClusterInfo()).getCloudClusterIdByName(
computeGroup);
// there are two cases can not find a cluster_id:
@@ -79,13 +80,17 @@ public class DropWorkloadGroupCommand extends DropCommand {
} else {
computeGroup = clusterId;
}
-
Env.getCurrentEnv().getWorkloadGroupMgr().dropWorkloadGroup(computeGroup,
workloadGroupName, ifExists);
} else {
+ // In non-cloud mode, 'FOR <compute_group>' is also supported
syntactically, but
+ // the value here actually refers to a resource group (Tag) —
there are no real
+ // compute groups in non-cloud mode. The grammar is shared with
cloud mode purely
+ // for consistency. When the clause is omitted, fall back to the
default resource
+ // group Tag.VALUE_DEFAULT_TAG.
if (StringUtils.isEmpty(computeGroup)) {
computeGroup = Tag.VALUE_DEFAULT_TAG;
}
-
Env.getCurrentEnv().getWorkloadGroupMgr().dropWorkloadGroup(computeGroup,
workloadGroupName, ifExists);
}
+
Env.getCurrentEnv().getWorkloadGroupMgr().dropWorkloadGroup(computeGroup,
workloadGroupName, ifExists);
}
@Override
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgr.java
b/fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgr.java
index fb1a9aa329e..2d3cd486f58 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgr.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgr.java
@@ -39,6 +39,7 @@ import org.apache.doris.thrift.TWorkloadActionType;
import org.apache.doris.thrift.TWorkloadMetricType;
import org.apache.doris.thrift.TopicInfo;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -412,7 +413,8 @@ public class WorkloadSchedPolicyMgr extends MasterDaemon
implements Writable, Gs
return ret;
}
- private void checkProperties(Map<String, String> properties, List<Long>
wgIdList) throws UserException {
+ @VisibleForTesting
+ void checkProperties(Map<String, String> properties, List<Long> wgIdList)
throws UserException {
Set<String> allInputPropKeySet = new HashSet<>();
allInputPropKeySet.addAll(properties.keySet());
@@ -443,20 +445,57 @@ public class WorkloadSchedPolicyMgr extends MasterDaemon
implements Writable, Gs
String workloadGroupNameStr =
properties.get(WorkloadSchedPolicy.WORKLOAD_GROUP);
if (workloadGroupNameStr != null && !workloadGroupNameStr.isEmpty()) {
- String cg = Config.isCloudMode() ?
Tag.VALUE_DEFAULT_COMPUTE_GROUP_NAME : Tag.VALUE_DEFAULT_TAG;
- String wg = "";
- String[] ss = workloadGroupNameStr.split("\\.");
- if (ss.length == 1) {
- wg = ss[0];
- } else if (ss.length == 2) {
+ // Use limit=-1 so trailing empty segments are preserved;
otherwise inputs like
+ // "wg." would silently collapse to ["wg"] and slip past the
length check, and
+ // ".wg" would pass the length==2 check with an empty
compute-group component.
+ String[] ss = workloadGroupNameStr.split("\\.", -1);
+ String cg;
+ String wg;
+ if (Config.isCloudMode()) {
+ // Cloud mode requires the fully-qualified
"<compute_group>.<workload_group>" form
+ // so the binding is unambiguous across multiple compute
groups.
+ if (ss.length != 2 || ss[0].isEmpty() || ss[1].isEmpty()) {
+ throw new UserException("workload_group must be
'<compute_group>.<workload_group>' "
+ + "in cloud mode, got: " + workloadGroupNameStr);
+ }
cg = ss[0];
wg = ss[1];
} else {
- throw new UserException("invalid workload group format: " +
workloadGroupNameStr);
+ // Non-cloud mode also accepts the
'<compute_group>.<workload_group>' form for
+ // grammar consistency with cloud mode, but the prefix here
actually refers to
+ // a resource group (Tag), not a real compute group. The bare
'<workload_group>'
+ // form is also accepted and defaults the resource group to
Tag.VALUE_DEFAULT_TAG.
+ if (ss.length == 1) {
+ if (ss[0].isEmpty()) {
+ throw new UserException("workload_group must be
'<workload_group>' or "
+ + "'<resource_group>.<workload_group>' in
non-cloud mode, got: "
+ + workloadGroupNameStr);
+ }
+ cg = Tag.VALUE_DEFAULT_TAG;
+ wg = ss[0];
+ } else if (ss.length == 2) {
+ if (ss[0].isEmpty() || ss[1].isEmpty()) {
+ throw new UserException("workload_group must be
'<workload_group>' or "
+ + "'<resource_group>.<workload_group>' in
non-cloud mode, got: "
+ + workloadGroupNameStr);
+ }
+ cg = ss[0];
+ wg = ss[1];
+ } else {
+ throw new UserException("workload_group must be
'<workload_group>' or "
+ + "'<resource_group>.<workload_group>' in
non-cloud mode, got: "
+ + workloadGroupNameStr);
+ }
}
ConnectContext tmpCtx = new ConnectContext();
tmpCtx.setComputeGroup(
Env.getCurrentEnv().getComputeGroupMgr().getComputeGroupByName(cg));
+ // In cloud mode ConnectContext#getComputeGroup() re-resolves the
compute group via
+ // getCloudCluster() and ignores setComputeGroup(), so propagate
the name through the
+ // session variable as well to make sure the downstream lookup
uses the chosen cg.
+ if (Config.isCloudMode()) {
+ tmpCtx.getSessionVariable().setCloudCluster(cg);
+ }
tmpCtx.getSessionVariable().setWorkloadGroup(wg);
tmpCtx.setCurrentUserIdentity(UserIdentity.ROOT);
Long wgId =
Env.getCurrentEnv().getWorkloadGroupMgr().getWorkloadGroup(tmpCtx)
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgrTest.java
b/fe/fe-core/src/test/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgrTest.java
new file mode 100644
index 00000000000..615fc0bc2eb
--- /dev/null
+++
b/fe/fe-core/src/test/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgrTest.java
@@ -0,0 +1,206 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.resource.workloadschedpolicy;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.UserException;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Unit tests for the workload_group property format enforced by
+ * {@link WorkloadSchedPolicyMgr#checkProperties(Map, List)}.
+ *
+ * The contract:
+ *
+ * - Cloud mode : workload_group must be
'<compute_group>.<workload_group>'.
+ * - Non-cloud mode: workload_group may be '<workload_group>' (defaulting the
+ * resource group to Tag.VALUE_DEFAULT_TAG) or the
+ * '<resource_group>.<workload_group>' form — the dotted
+ * prefix is a resource group (Tag) here, sharing the
cloud-mode
+ * grammar purely for consistency.
+ *
+ * Invalid forms must be rejected BEFORE any compute-group lookup, so the
+ * rejection is exercisable here without bootstrapping the full Env.
+ */
+public class WorkloadSchedPolicyMgrTest {
+
+ private String originDeployMode;
+ private String originCloudUniqueId;
+ private WorkloadSchedPolicyMgr mgr;
+
+ @Before
+ public void setUp() {
+ originDeployMode = Config.deploy_mode;
+ originCloudUniqueId = Config.cloud_unique_id;
+ mgr = new WorkloadSchedPolicyMgr();
+ }
+
+ @After
+ public void tearDown() {
+ Config.deploy_mode = originDeployMode;
+ Config.cloud_unique_id = originCloudUniqueId;
+ }
+
+ private Map<String, String> propsWith(String workloadGroupValue) {
+ Map<String, String> p = new HashMap<>();
+ p.put(WorkloadSchedPolicy.WORKLOAD_GROUP, workloadGroupValue);
+ return p;
+ }
+
+ @Test
+ public void testCloudModeRejectsUnqualifiedWorkloadGroup() {
+ Config.cloud_unique_id = "ut_cloud";
+ Assert.assertTrue(Config.isCloudMode());
+
+ try {
+ mgr.checkProperties(propsWith("superset"), new ArrayList<>());
+ Assert.fail("expected UserException for unqualified workload_group
in cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention
<compute_group>.<workload_group>; got: " + e.getMessage(),
+
e.getMessage().contains("<compute_group>.<workload_group>"));
+ Assert.assertTrue("message should mention cloud mode; got: " +
e.getMessage(),
+ e.getMessage().contains("cloud mode"));
+ }
+ }
+
+ @Test
+ public void testCloudModeRejectsTooManyDotsInWorkloadGroup() {
+ Config.cloud_unique_id = "ut_cloud";
+ Assert.assertTrue(Config.isCloudMode());
+
+ try {
+ mgr.checkProperties(propsWith("etl.superset.extra"), new
ArrayList<>());
+ Assert.fail("expected UserException for over-qualified
workload_group in cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention
<compute_group>.<workload_group>; got: " + e.getMessage(),
+
e.getMessage().contains("<compute_group>.<workload_group>"));
+ }
+ }
+
+ @Test
+ public void testNonCloudModeRejectsTooManyDotsInWorkloadGroup() {
+ // The '<resource_group>.<workload_group>' form is allowed in
non-cloud mode, but
+ // anything with more than one dot is still ambiguous and must be
rejected before
+ // any lookup.
+ Config.deploy_mode = "share_nothing";
+ Config.cloud_unique_id = "";
+ Assert.assertFalse(Config.isCloudMode());
+
+ try {
+ mgr.checkProperties(propsWith("etl.superset.extra"), new
ArrayList<>());
+ Assert.fail("expected UserException for over-qualified
workload_group in non-cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention the allowed forms; got:
" + e.getMessage(),
+ e.getMessage().contains("<workload_group>"));
+ Assert.assertTrue("message should mention non-cloud mode; got: " +
e.getMessage(),
+ e.getMessage().contains("non-cloud mode"));
+ }
+ }
+
+ @Test
+ public void testEmptyOrMissingWorkloadGroupPropertyIsAccepted() throws
Exception {
+ Config.cloud_unique_id = "ut_cloud";
+ Assert.assertTrue(Config.isCloudMode());
+
+ // Absent property is OK: the workload_group binding is simply not set.
+ mgr.checkProperties(new HashMap<>(), new ArrayList<>());
+
+ // Explicit empty string is OK too: ignored like absent.
+ mgr.checkProperties(propsWith(""), new ArrayList<>());
+ }
+
+ @Test
+ public void testCloudModeRejectsTrailingDotInWorkloadGroup() {
+ Config.cloud_unique_id = "ut_cloud";
+ Assert.assertTrue(Config.isCloudMode());
+
+ // "etl." splits to ["etl", ""] under split(".", -1); the empty
workload-group
+ // segment must be rejected before reaching the compute-group lookup.
+ try {
+ mgr.checkProperties(propsWith("etl."), new ArrayList<>());
+ Assert.fail("expected UserException for trailing-dot
workload_group in cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention
<compute_group>.<workload_group>; got: " + e.getMessage(),
+
e.getMessage().contains("<compute_group>.<workload_group>"));
+ }
+ }
+
+ @Test
+ public void testCloudModeRejectsLeadingDotInWorkloadGroup() {
+ Config.cloud_unique_id = "ut_cloud";
+ Assert.assertTrue(Config.isCloudMode());
+
+ // ".superset" splits to ["", "superset"]; the empty compute-group
segment must
+ // be rejected rather than falling through with an empty cg name.
+ try {
+ mgr.checkProperties(propsWith(".superset"), new ArrayList<>());
+ Assert.fail("expected UserException for leading-dot workload_group
in cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention
<compute_group>.<workload_group>; got: " + e.getMessage(),
+
e.getMessage().contains("<compute_group>.<workload_group>"));
+ }
+ }
+
+ @Test
+ public void testNonCloudModeRejectsTrailingDotInWorkloadGroup() {
+ Config.deploy_mode = "share_nothing";
+ Config.cloud_unique_id = "";
+ Assert.assertFalse(Config.isCloudMode());
+
+ // "wg." splits to ["wg", ""]; previously split("\\.") would drop the
trailing
+ // empty segment and let this pass. With split(..., -1) the empty
workload-group
+ // component is detected and rejected before lookup.
+ try {
+ mgr.checkProperties(propsWith("wg."), new ArrayList<>());
+ Assert.fail("expected UserException for trailing-dot
workload_group in non-cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention the allowed forms; got:
" + e.getMessage(),
+ e.getMessage().contains("<workload_group>"));
+ Assert.assertTrue("message should mention non-cloud mode; got: " +
e.getMessage(),
+ e.getMessage().contains("non-cloud mode"));
+ }
+ }
+
+ @Test
+ public void testNonCloudModeRejectsLeadingDotInWorkloadGroup() {
+ Config.deploy_mode = "share_nothing";
+ Config.cloud_unique_id = "";
+ Assert.assertFalse(Config.isCloudMode());
+
+ // ".wg" splits to ["", "wg"]; the empty resource-group component must
be
+ // rejected rather than falling through with an empty cg name.
+ try {
+ mgr.checkProperties(propsWith(".wg"), new ArrayList<>());
+ Assert.fail("expected UserException for leading-dot workload_group
in non-cloud mode");
+ } catch (UserException e) {
+ Assert.assertTrue("message should mention the allowed forms; got:
" + e.getMessage(),
+ e.getMessage().contains("<workload_group>"));
+ Assert.assertTrue("message should mention non-cloud mode; got: " +
e.getMessage(),
+ e.getMessage().contains("non-cloud mode"));
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
