morningman commented on PR #70130: URL: https://github.com/apache/doris/pull/70130#issuecomment-6622257343
Review result: **REQUEST_CHANGES** (1 Blocker, 0 Major, 0 Minor, 0 Nit). Reviewed commit: `72e91896bbdd8760f8bd4e488e5672406b40ce52`, using GPT-6.1 Sol (`xhigh`). **F-01: Create the regression workload group in the job's compute/resource groups** - Severity: **Blocker** (test fixture failure) - Category: `functional-bug` - Regression: **yes** - Location: [test_routine_load_alter.groovy:235-238](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/regression-test/suites/load_p0/routine_load/test_routine_load_alter.groovy#L235-L238) ```groovy def validCluster = clusters[0][0] forComputeGroupStr = " for $validCluster " } sql "create workload group if not exists alter_routine_load_group $forComputeGroupStr properties ( 'min_cpu_percent'='0');" ``` The fixture can create `alter_routine_load_group` in a different namespace from the one used by the routine-load job. The new validation then rejects the existing positive ALTER, so the suite stops before its remaining assertions. Two supported configurations expose this when Kafka tests are enabled and the workload group does not already exist in the required namespace: 1. **Cloud:** the user can access healthy `cg_a` and `cg_b`, but the current/default group is `cg_b`. [SHOW CLUSTERS sorts the inventory by name and reports the active group separately](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowClustersCommand.java#L111-L132). Row zero selects `cg_a`, while [the ALTER check uses the job's effective group](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/load/routineload/RoutineLoadJob.java#L2131-L2132), `cg_b`. The positive ALTER fails with `Can not find workload group alter_routine_load_group in compute croup cg_b`. 2. **Non-cloud:** an untagged root/admin owner has healthy default and extra-tag backends. [Omitting FOR creates the workload group only in default](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateWorkloadGroupCommand.java#L110-L129), while [AllBackendComputeGroup requires it in every backend tag](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/resource/computegroup/AllBackendComputeGroup.java#L79-L100). The extra-tag lookup rejects the positive ALTER. At merge base `856194ece0e1e0ad45b46b96f26a3e2859412e06`, Kafka checked workload groups only when ALTER also declared `compute_group`, so the same workload-group-only positive statement reached its assertions. The production validation change is intended; the positive test fixture needs to satisfy it. The severity refers to a broken test contract, with no claim of production data corruption. Suggested fix: select the active cloud group, and create the non-cloud fixture in each owner-visible resource group. For example: ```diff - def forComputeGroupStr = "" - if (isCloudMode()) { - def clusters = sql " SHOW CLUSTERS; " - assertTrue(!clusters.isEmpty()) - def validCluster = clusters[0][0] - forComputeGroupStr = " for $validCluster " + def clusters = sql "SHOW CLUSTERS;" + assertTrue(!clusters.isEmpty()) + def targetGroups = isCloudMode() ? clusters.findAll { it[1] == "TRUE" } : clusters + assertTrue(!targetGroups.isEmpty()) + targetGroups.each { group -> + sql "create workload group if not exists alter_routine_load_group for ${group[0]} properties ( 'min_cpu_percent'='0');" } - sql "create workload group if not exists alter_routine_load_group $forComputeGroupStr properties ( 'min_cpu_percent'='0');" ``` Review scope: all six changed files and related invocation paths, with two review rounds. No additional production finding was identified. This is source-based review: no build or test was run, and the suggested patch has not been executed. -- 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]
