morningman opened a new pull request, #70130:
URL: https://github.com/apache/doris/pull/70130
### What problem does this PR solve?
Issue Number: None
Related PR: #69749, #50563, #49960, #53490
Problem Summary:
**In short.** `ALTER ROUTINE LOAD ... PROPERTIES("workload_group" = "...")`
accepts any name. `CREATE ROUTINE LOAD` checks the workload group as the job's
owner in the compute group the job runs in, and every task of the job resolves
it the same way, so a name that is missing there, or that the owner may not
use, is accepted by the ALTER and then fails every task. This PR checks it at
ALTER time, as CREATE does.
**Background.** A workload group belongs to a compute group in cloud mode,
and to the owner's resource group outside it. Each task looks the job's
workload group up as the owner in the job's compute group
(`KafkaTaskInfo#rePlan`, `KinesisTaskInfo#rePlan`). Kafka jobs used to check an
altered workload group (#50563, in the `AlterRoutineLoadStmt` path). The
Nereids `AlterRoutineLoadCommand` path (#49960) did not carry that check, and
#53490 removed the old path. #69749 brought a check back only for an ALTER that
moves a Kafka job to another compute group. The comment that delays the check
("we can only get clusterId when alter job") is no longer true: the ALTER has
the job at hand. Found while reviewing #69749 (thread 4415694066).
**The problem, and what it cost.** Measured on master `856194ece0e` with
unit tests; each ALTER below goes straight to the PAUSED check, so on a paused
job it is accepted and written to the edit log:
| Job | ALTER | Result |
|---|---|---|
| Kafka, cloud mode, runs in `cg_a` | `"workload_group" = "wg_b"`, which
only `cg_b` has | accepted |
| Kafka, outside cloud mode | a workload group that does not exist |
accepted |
| Kinesis, outside cloud mode | a workload group that does not exist |
accepted |
After RESUME, the first task of each such job fails with `Can not find
workload group ...`, and the job pauses with `CREATE_TASKS_ERR`. That code is
retryable, so auto resume repeats this with backoff (20 s up to 300 s) for as
long as the job exists. The ALTER that caused it reported success.
**How this PR fixes it.**
- `RoutineLoadJob#checkAlteredWorkloadGroup`, called by the Kafka and the
Kinesis job before they take the job lock. When an ALTER sets a workload group
or declares a compute group, it checks the resulting workload group in the
resulting compute group, with `CreateRoutineLoadInfo.checkWorkloadGroup` as
CREATE does. The resulting workload group is the one the ALTER sets, else the
one the job keeps; the resulting compute group is the one the ALTER declares,
else the job's. The check runs as the owner, because the tasks run as the
owner. A rejected ALTER changes nothing and writes no edit log. The replay of
an ALTER does not go through this path.
- The Kafka job's check for a move to another compute group (#69749) is
replaced by this one; it checks the same thing for that ALTER.
- The comments in `AlterRoutineLoadCommand` that pointed at the old place,
or at the old reason for not checking, are corrected.
- `test_routine_load_alter` altered a job to `alter_routine_load_group`
without creating the group: it worked only because the ALTER did not check, or
because `test_routine_load` happened to create the group first. It now creates
the group, the way `test_routine_load` does, and checks that an ALTER to a
missing group fails.
**Results.**
| ALTER | Before | After |
|---|---|---|
| a workload group the job's compute group has, and the owner may use |
accepted | accepted |
| a workload group the job's compute group lacks, or the owner may not use |
accepted, then every task fails and auto resume loops | rejected, e.g. `Can not
find workload group wg_b in compute croup cg_a`; the job is unchanged |
| a compute group, keeping a workload group the new group lacks (Kafka) |
rejected (#69749) | rejected |
| neither property | not checked | not checked |
branch-4.1 and branch-4.2 have the same gap.
### Release note
ALTER ROUTINE LOAD now rejects a workload_group that does not exist in the
job's compute group (outside cloud mode, the job owner's resource group), or
that the job's owner may not use, as CREATE ROUTINE LOAD does. Such an ALTER
used to succeed, and then every task of the job failed.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`RoutineLoadComputeGroupTest` gets 2 cases. In cloud mode, a
workload-group-only ALTER is checked in the job's compute group, both for a job
running in the group snapshotted at creation and for a job that declares its
group. Outside cloud mode, Kafka and Kinesis jobs are checked, and every lookup
runs as the owner. The workload group mock moves into a shared helper. One case
of #69749 now asserts that an ALTER setting neither property looks nothing up,
instead of asserting that a workload-group-only ALTER looks nothing up. Both
new cases fail on master. 11 routine load test classes, 107 cases, pass. Each
part of the fix, broken on its own, fails its case: leaving the Kinesis job
unchecked, checking a move in the job's old compute group, checking only an
ALTER that declares a compute group, checking as whoever runs the ALTER instead
of the owner, and ignoring the workload group the ALTER sets. The
`test_routine_load_alter` change needs Kafka, so it was only parsed offline and
w
as not run locally.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
`ALTER ROUTINE LOAD` rejects a workload group that the job could not
use, which it used to accept. A workload group now has to be created, and
granted to the job's owner, before a job is altered to it, as before `CREATE
ROUTINE LOAD`.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
--
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]
