github-actions[bot] commented on code in PR #60559:
URL: https://github.com/apache/doris/pull/60559#discussion_r2986728131
##########
regression-test/suites/workload_manager_p0/test_workload_sched_policy.groovy:
##########
@@ -247,4 +247,43 @@ suite("test_workload_sched_policy") {
sql "drop user test_alter_policy_user"
sql "drop workload policy test_alter_policy"
+
+ //
============================================================================
+ // Test mixed policy (Username + Query Time)
+ //
============================================================================
+
+ // 1. Create a user
+ sql "DROP USER IF EXISTS 'test_policy_user_be'"
+ sql "CREATE USER 'test_policy_user_be'@'%' IDENTIFIED BY '12345'"
+ sql "GRANT SELECT_PRIV ON *.* TO 'test_policy_user_be'@'%'"
+
+ // 2. Create a workload group
+ sql "DROP WORKLOAD GROUP IF EXISTS policy_group_be $forComputeGroupStr"
+ sql "CREATE WORKLOAD GROUP policy_group_be $forComputeGroupStr PROPERTIES
('max_cpu_percent'='100')"
+ sql "GRANT USAGE_PRIV ON WORKLOAD GROUP 'policy_group_be' TO
'test_policy_user_be'@'%'"
+
+ // 3. Create a policy with both username (FE metric) and query_time (BE
metric)
+ sql "DROP WORKLOAD POLICY IF EXISTS test_mixed_policy"
Review Comment:
Nit: The comment says `username (FE metric)` but after this PR, USERNAME is
a shared (FE+BE) metric. This comment is misleading since the whole point of
this test is that USERNAME can now coexist with BE metrics.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/runtime/ThriftPlansBuilder.java:
##########
@@ -408,6 +409,13 @@ private static TPipelineFragmentParams
fragmentToThriftIfAbsent(
params.setLocalParams(Lists.newArrayList());
params.setWorkloadGroups(coordinatorContext.getWorkloadGroups());
+ if (connectContext != null &&
connectContext.getCurrentUserIdentity() != null) {
+ TResourceInfo resourceInfo = new TResourceInfo();
Review Comment:
**[Issue - Parallel Code Paths]** `resourceInfo` is only set here in the
Nereids `ThriftPlansBuilder` path. There are at least two other paths that
build `TPipelineFragmentParams` but do NOT set `resourceInfo`:
1. **Legacy Coordinator**: `Coordinator.java` →
`FragmentExecParams.toThrift()` (around line 3247) — builds
`TPipelineFragmentParams` without setting `resourceInfo`.
2. **NereidsStreamLoadPlanner**: `NereidsStreamLoadPlanner.java` → `plan()`
— used for stream loads, routine loads, and group commits — also does not set
`resourceInfo`.
This means username-based BE workload policies will **not apply** to:
- Queries going through the legacy (non-Nereids) coordinator path
- Stream loads, routine loads, and group commit loads
For those paths, `query_ctx->set_rsc_info` will remain `false`, so
`QueryTaskController::get_user()` will always return `""`, and USERNAME
conditions will never match.
Please consider whether these paths should also propagate `resourceInfo`, or
at minimum document this limitation.
##########
fe/fe-core/src/main/java/org/apache/doris/resource/workloadschedpolicy/WorkloadSchedPolicyMgr.java:
##########
@@ -105,15 +105,16 @@ public WorkloadSchedPolicyMgr() {
public static final ImmutableSet<WorkloadMetricType> BE_METRIC_SET
= new
ImmutableSet.Builder<WorkloadMetricType>().add(WorkloadMetricType.BE_SCAN_ROWS)
.add(WorkloadMetricType.BE_SCAN_BYTES).add(WorkloadMetricType.QUERY_TIME)
- .add(WorkloadMetricType.QUERY_BE_MEMORY_BYTES).build();
+
.add(WorkloadMetricType.QUERY_BE_MEMORY_BYTES).add(WorkloadMetricType.USERNAME).build();
Review Comment:
**[Issue - Missing Operator Validation for USERNAME]** Adding `USERNAME` to
`BE_METRIC_SET` makes it a shared (FE+BE) metric. However, there is no
validation anywhere that restricts operators for `USERNAME` to `EQUAL` only.
A user can create a policy like:
```sql
CREATE WORKLOAD POLICY test CONDITIONS(username > 'admin')
ACTIONS(cancel_query)
```
This will:
- **Parse and persist successfully** (grammar allows all comparison
operators)
- **On FE evaluation**: `WorkloadConditionCompareUtils.compareString()`
throws `RuntimeException("unexpected compare string operator GREATER")` — this
crashes the FE scheduler daemon thread
- **On BE evaluation**: `WorkloadCompareUtils::compare_string()` logs an
error and returns `false` — condition silently never matches
Recommendation: Add validation in
`WorkloadConditionUsername.createWorkloadCondition()` (FE side) to reject
non-EQUAL operators with a `UserException` at policy creation time, preventing
invalid policies from being persisted.
--
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]
