morningman commented on code in PR #69749:
URL: https://github.com/apache/doris/pull/69749#discussion_r4419959806
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterRoutineLoadCommand.java:
##########
@@ -287,6 +289,18 @@ private void checkJobProperties() throws UserException {
analyzedJobProperties.put(CreateRoutineLoadInfo.UNIQUE_KEY_UPDATE_MODE,
modeStr.toUpperCase());
}
+ if (jobProperties.containsKey(CreateRoutineLoadInfo.COMPUTE_GROUP)) {
+ String computeGroup =
jobProperties.get(CreateRoutineLoadInfo.COMPUTE_GROUP);
+ if (!StringUtil.isEmpty(computeGroup)) {
+ // Unlike workload group, the compute group can be fully
validated right here, so do
+ // it now instead of letting a bad name pause the whole job on
the next task.
Review Comment:
Fixed in `c9d9730047e`. An ALTER that sets `compute_group` now checks the
job's workload group in that group, as CREATE does in the group it declares.
`KafkaRoutineLoadJob.modifyProperties` resolves the workload group the job will
have afterwards, which is the one the ALTER sets or else the one the job keeps.
The lookup runs as the owner the tasks run as, on the statement thread before
the job lock, and the ALTER is rejected when the group does not have that
workload group. It is the lookup CREATE uses, now shared as
`CreateRoutineLoadInfo.checkWorkloadGroup`. The `AlterRoutineLoadCommand`
comment that said the workload group could not be checked at ALTER was wrong:
the job is known there, and #50563 checked its workload group in the job's
group in the old `modifyProperties(AlterRoutineLoadStmt)`. The Nereids
`modifyProperties(AlterRoutineLoadCommand)` from #49960 never carried that
check, and #53490 removed the statement path. As a result, an ALTER that sets
`workload_group` alon
e is still unchecked on master and branch-4.x. That predates this PR and is
unchanged here, as is `DROP WORKLOAD GROUP`, which does not consider routine
load jobs either. The new
`RoutineLoadComputeGroupTest#testAlterChecksWorkloadGroupInTheComputeGroupItDeclares`
covers three cases: a move that keeps a workload group the new group lacks, a
move that names a workload group of the new group, and an ALTER without
`compute_group`. It fails without the check, and also when the check runs in
the job's current group.
##########
fe/fe-core/src/main/java/org/apache/doris/load/routineload/RoutineLoadJob.java:
##########
@@ -1488,6 +1509,20 @@ public void updateState(JobState jobState, ErrorReason
reason, boolean isReplay)
.build());
}
+ // For failures found by refreshKafkaPartitions(), which update() runs
before it takes the job lock.
+ // The state is checked again under the lock: a STOP that completed in
between must not be turned
Review Comment:
Fixed in `c9d9730047e`. Confirmed: auto resume runs the partition refresh
while the job is still PAUSED, and `pauseIfNeedSchedule()` only pauses a
NEED_SCHEDULE job (PAUSED -> PAUSED is rejected, on replay too), so a failing
check there kept the old, retryable reason. This came with the refresh check in
`4bf82ff4a9d`. Before that commit, and on branch-4.x, which has no check in the
refresh, the attempt reached the task scheduler's check and recorded
CANNOT_RESUME_ERR. The refresh now skips the compute group check for an auto
resume attempt. Once the metadata request succeeds, the attempt moves the job
to NEED_SCHEDULE, and the check `RoutineLoadTaskScheduler` runs before the
first task pauses the job with CANNOT_RESUME_ERR under the job lock, which
stops auto resume. A manual RESUME leaves the job in NEED_SCHEDULE, so the
refresh still runs the check then, and a dropped group is still reported as
`Compute group '<name>' not found.`. The comments at the check and at
`pauseIfNeedSch
edule()` now state this contract. The new
`RoutineLoadComputeGroupTest#testAutoResumeLeavesRevokedUsageToTheTaskCheck`
drives `refreshKafkaPartitions(true)` for a paused job whose owner lost USAGE,
then schedules its first task. It checks that the job ends up paused with
CANNOT_RESUME_ERR and is not auto resumed, and it fails when the refresh runs
the check for the attempt.
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1680,6 +1684,34 @@ private void setupComputeGroup(ConnectContext ctx) {
}
}
Review Comment:
Fixed in `c9d9730047e`. The race is real but narrow, and its effect is
smaller than described. The refresh ran in the group that was declared when its
context was bound, which is a valid order for an ALTER that lands meanwhile.
The only problem was that the re-check could look at the new group instead.
That shows up when the old group is dropped at the same time: the refresh then
failed later, at backend selection, with a generic error instead of `Compute
group '<name>' not found.`. No data was affected. `setupComputeGroup()` now
reads the declaration once, binds and records it, and re-checks that same
value. Both refresh paths go through it, and the `REFRESH ... WITH DRY RUN`
context already worked this way. The new
`MTMVComputeGroupTest#testRefreshContextChecksTheGroupItBinds` makes the first
read return a dropped group and any later read an existing one. It expects a
single read and a check that fails for the bound group, and it fails when the
check reads the property again.
--
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]
