[feature](routineload) Add routine load target-table alter support#64878
Open
0AyanamiRei wants to merge 20 commits into
Open
[feature](routineload) Add routine load target-table alter support#648780AyanamiRei wants to merge 20 commits into
0AyanamiRei wants to merge 20 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Allow paused single-table Routine Load jobs to switch their target table with ALTER ROUTINE LOAD FOR [db.]job ON <table>, while preserving existing progress and replaying the new table binding from edit log. ### Release note Support ALTER ROUTINE LOAD ... ON <table> to switch the target table for paused single-table routine load jobs. ### Check List (For Author) - Test: FE unit test - "/data/data3/huangruixin/include/src-master/apache-maven-3.9.9/bin/mvn -pl fe-core -am -DskipITs -Dcheckstyle.skip=true -DfailIfNoTests=false -Dtest=org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest test" - Kinesis unit tests skipped per user request - Behavior changed: Yes (new ALTER ROUTINE LOAD target-table switch behavior) - Does this need documentation: Yes (documented in /data/data3/huangruixin/docs/routine-load-alter-table-design.html)
0AyanamiRei
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
morrySnow,
mymeiyi and
starocean999
as code owners
June 26, 2026 04:59
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
0AyanamiRei
marked this pull request as draft
June 26, 2026 06:04
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: Follow-up review found two issues in the new routine load target-table alter support. First, `AlterRoutineLoadCommand` had an import order regression that could fail FE checkstyle. Second, alter validation only rechecked `PARTIAL_COLUMNS=true` from the current command, which left the effective partial-update state under-validated when the existing job or a `unique_key_update_mode` change required merge-on-write semantics. This change restores import ordering, validates the effective unique key update mode against the destination table, and adds focused FE unit coverage for those cases. ### Release note Routine Load alter now rejects target-table or unique-key-update changes that are incompatible with partial update requirements. ### Check List (For Author) - Test: Unit Test - Behavior changed: Yes - Does this need documentation: No
0AyanamiRei
marked this pull request as ready for review
June 29, 2026 01:11
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: The routine load target-table alter change hit FE checkstyle in CI because one validation error message exceeded the line-length limit and a unit-test import order did not match the FE custom import ordering rule. This commit makes the minimal formatting-only fixes so the branch aligns with FE style checks. ### Release note None ### Check List (For Author) - Test: No need to test (formatting-only fix requested by reviewer; no local build or test run) - Behavior changed: No - Does this need documentation: No
0AyanamiRei
changed the title
Contributor
|
/review |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: The routine load target-table alter implementation had a single-use helper for constructing the validation load descriptor snapshot, and the parser branch for table-only alter did not mark the intended phase-one scope. This commit inlines the one-off snapshot construction at the validation call site and documents that the current parser branch only supports target table alteration before future support for combining target-table and property changes. ### Release note None ### Check List (For Author) - Test: No need to test (review follow-up only; no local build or test run) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed for PR 64878 at head 760890d.
I found one correctness issue: validation can accept an alter that later leaves the routine-load job in partial-update mode on a non-MoW table.
Critical checkpoints:
- Goal: the PR adds single-table routine-load target-table alteration while preserving stream progress; the implementation largely wires parser, command validation, job mutation, replay, display, and tests, but the partial-update validation path is inconsistent with mutation for one property combination.
- Scope/focus: the target-table change is small and localized to routine-load command/job/persistence paths.
- Concurrency/lifecycle: mutation is still gated by the existing paused-job write lock; no new background thread or lock order was introduced.
- Configuration/session propagation: no new config item; existing routine-load session/job properties are reused.
- Compatibility/persistence: new log field is backward-readable by current code; target-table id is replayed in Kafka and Kinesis paths.
- Parallel paths: Kafka and Kinesis both update/replay targetTableId; scheduling/task planning uses the current job tableId.
- Tests: parser/command/persistence/unit coverage and a Kafka regression were added. I verified
git diff --checkandmvn -pl fe-core -DskipTests checkstyle:check. I did not run FE compile/unit/regression tests because this checkout lacksthirdparty/installed/bin/protocandthirdparty/installed. - User focus: no additional user-provided focus was present.
Subagent conclusions: optimizer-rewrite proposed OPT-001, which became inline comment MAIN-001. tests-session-config reported no distinct candidate. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29129 ms |
Contributor
TPC-DS: Total hot run time: 171607 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: ALTER ROUTINE LOAD validation handled unique_key_update_mode and legacy partial_columns with different precedence from the mutation path. An ALTER containing unique_key_update_mode=UPSERT and partial_columns=true could pass validation on a non-MoW table, then be applied as UPDATE_FIXED_COLUMNS. Flexible partial-update ALTERs on non-MoW tables were also rejected by the generic PARTIAL_COLUMNS validation before reaching the flexible partial-update validation path. This change makes generic partial-column validation apply only to fixed partial update mode and makes the mutation path ignore legacy partial_columns when an explicit unique_key_update_mode is present. The routine load alter regression test also declares its polling counters as local variables to satisfy regression framework script checks. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Review completed for PR 64878 at head 8b19f60c7a9c973cb565b6963e76cfdf836a24d2.
I found five new P1 correctness issues and am requesting changes. I did not repeat the existing threads covering target authorization/identity races, ALTER execution context, Kafka datetime-origin replacement, or the resolved metadata-lock I/O concern.
Critical checkpoints:
- Goal: the PR adds composable paused-job target switching with job/source-property changes and durable replay. The parser and happy path are present, but the five inline issues show data-position loss, rejected-ALTER side effects, stale-target concurrency, and failover divergence, so the goal is not yet safely met.
- Scope/focus: the 16 changed paths form a focused end-to-end FE/parser/persistence/test change. No unrelated production change was found.
- Concurrency:
tableId, load descriptors, and source state are shared across concurrent ALTER/control/SHOW/update work. Job/database/table lock order showed no concrete deadlock, but MAIN-007 proves descriptor validation and application are not bound to one target version. Kafka/MetaService preparation is outside metadata locks, but MAIN-006 shows that the externally committed side effect is no longer atomic with final validation. - Lifecycle: target switching is limited to PAUSED jobs and existing task clearing prevents simultaneous old/new-target tasks. Resume, dynamic partition/shard discovery, progress refresh, and follower promotion were reviewed; MAIN-002, MAIN-007, and MAIN-008 fail at those lifecycle boundaries. No static/global initialization or release issue applies.
- Configuration: no configuration item or dynamic-config behavior is added.
- Compatibility: legacy records safely default missing
targetTableIdto zero, and the documented follower-first/master-last FE upgrade order excludes the dismissed old-reader election scenario. Same-version replay is still unsafe under stale Cloud progress (MAIN-008). - Parallel paths: Kafka and Kinesis ALTER analysis/live/replay/image behavior were compared. Kafka-only target switching is intentionally enforced; Kinesis still misses the promised unspecified-default preservation (MAIN-002).
- Special conditions: paused state, single-table/Kafka type, target type/ID/schema/distribution, partial-update mode, pinned/unpinned partitions, numeric/symbolic/time offsets, and Cloud/non-Cloud branches were checked. MAIN-005 is the new all-numeric/mixed numeric partition-membership hole.
- Test coverage: changed parser, command, persistence, job unit tests, and the ordered Kafka regression cover syntax and success paths. They omit the Kinesis default case, invalid numeric partition, post-reset target failure, two-ALTER descriptor race, and stale-follower replay represented by the five comments.
- Test results: no local build or test was run because this review contract forbids builds. Current CI has compile, FE/BE/Cloud unit tests, cloud/vault regressions, coverage, formatting, and license checks passing; P0, external, and nonConcurrent regressions remain pending, while the performance job is failing.
- Observability: existing routine-load and replay logs expose errors, but logging and swallowing the replay exception does not preserve state consistency (MAIN-008). No separate metric/logging blocker was substantiated.
- Persistence/failover: EditLog/image/legacy-record paths were traced. MAIN-008 is a same-version replay divergence; existing context and mutable-identity threads remain applicable and were not duplicated.
- Data modification/atomicity: no BE row-write implementation changes, but these offsets control which rows are ingested. MAIN-002 and MAIN-005 can skip records, and MAIN-006 lets an uncommitted/rejected FE ALTER durably mutate MetaService progress.
- FE/BE propagation: no new FE-to-BE variable or protocol field is added; task planning and transaction setup consume the job's current
tableId. - Performance: no new CPU, memory, or hot-path complexity issue was found. The earlier external-I/O-under-metadata-lock concern is already covered by
discussion_r3600083763and was not repeated. - Other/user focus: no additional user-provided focus was present. Two complete review rounds converged; both complementary full-review agents and the separate risk reviewer returned
NO_NEW_VALUABLE_FINDINGSfor this exact five-comment set and anchors.
| } finally { | ||
| writeUnlock(); | ||
| } | ||
| return () -> modifyPropertiesInternal(jobProperties, dataSourceProperties); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the stored Kinesis default when this ALTER delta omits one. KinesisDataSourceProperties still calls analyzeKinesisDefaultPositionProperty() for every ALTER without explicit positions, which inserts kinesis_default_pos=LATEST into the delta. This closure feeds that synthetic value to customProperties.putAll(), so a stream/region/endpoint/custom-only ALTER overwrites an existing TRIM_HORIZON; on a stream switch, initial shard discovery then starts at LATEST and skips every record already present. Mirror Kafka's ALTER early return when neither shards nor a default was supplied, and cover live/replay/image preservation.
| if (MapUtils.isNotEmpty(dataSourceProperties.getOriginalDataSourceProperties())) { | ||
| kafkaPartitionOffsets = dataSourceProperties.getKafkaPartitionOffsets(); | ||
| } | ||
| if (!kafkaPartitionOffsets.isEmpty() |
Contributor
There was a problem hiding this comment.
[P1] Validate numeric-offset partitions against the staged topic. For an unpinned topic switch this condition skips the old-progress check, and resolveOffsets() cannot replace it: KafkaUtil.getRealOffsets() returns an all-numeric list without contacting Kafka. ALTER ... kafka_topic=new, kafka_partitions=999, kafka_offsets=0 therefore succeeds and journals bogus progress even when new has only partition 0; dynamic discovery later ignores 999 and starts real partitions from the default. Fetch the staged topic's partition metadata for every requested ID, including all-numeric and mixed lists, before the Cloud reset or local apply.
|
|
||
| if (!isReplay) { | ||
| kafkaPartitionOffsets = resolveOffsets(dataSourceProperties, dataSourceSnapshot); | ||
| resetCloudProgressIfNeeded(dataSourceSnapshot, kafkaPartitionOffsets); |
Contributor
There was a problem hiding this comment.
[P1] Do not commit Cloud progress during preparation. This RPC runs before the new target is re-resolved/replanned and before local apply or the EditLog write. In a combined target/source ALTER, another session can drop the analyzed target or make its schema incompatible; the reset commits, final validation then throws, and the FE job/journal stay unchanged while MetaService contains the new offsets. A later progress refresh or failover can import them into the old job and skip or replay records. Use a durable two-phase/conditional protocol or compensation so a rejected ALTER cannot change MetaService.
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: Kafka routine load ALTER could keep a stale datetime origin when a symbolic default offset was supplied, reset Cloud MetaService progress before final metadata validation and edit-log persistence, and replay resolved ALTER state against follower-local progress. Routine load descriptors were also applied outside the Kafka job lock and were absent from the ALTER journal. Restore the pre-existing Kinesis ALTER implementation, keep the new preparation flow Kafka-only, persist pending Cloud progress changes until the job resumes, make replay deterministic, and journal descriptor changes with the target-table mutation. ### Release note Fix Kafka routine load ALTER consistency for default offsets, Cloud progress, target-table changes, and replay. ### Check List (For Author) - Test: FE unit tests and FE build - `./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.load.routineload.RoutineLoadJobTest,org.apache.doris.load.routineload.RoutineLoadManagerTest,org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest` - `./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest` - `./build.sh --fe -j48` - Behavior changed: Yes. Cloud progress changes are submitted when the paused Kafka job resumes, after ALTER validation and persistence; symbolic default offsets now replace an earlier datetime default correctly. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two distinct PR-introduced P1 issues remain in the inline comments: checkpoint reload can cancel a valid target/property transition, and target ALTER can wait synchronously for the journal while holding database/table metadata locks.
Critical checkpoint conclusions:
- Goal and scope: The 16-file FE/parser/test change is focused on composable paused single-table routine-load target, job, load-clause, and Kafka source ALTERs. The primary live paths are covered, but the two blockers mean the goal is not restart- and availability-safe yet.
- Concurrency and locks: The job-write -> database-read -> table-read order is consistent, and Kafka/MetaService preparation was moved outside metadata locks. The remaining synchronous EditLog wait under those locks is a blocking availability defect; other target/authorization/descriptor races are already covered by existing threads and were not duplicated.
- Lifecycle: The pending Cloud progress-reset state was traced across ALTER, RESUME, replay, image load, failover, retries, and both reset RPC boundaries and is reconstructible/idempotent. No static/global lifetime issue applies. Image post-processing still introduces the checkpoint-cancellation defect.
- Configuration and compatibility: No configuration item or FE-BE protocol variable is added. Old journal records safely default the new target/descriptor fields, and parser compatibility/intentional exclusions have tests; current job-image behavior is not compatible with the newly mutable target.
- Parallel paths and conditions: Kafka/Kinesis, UPSERT/fixed/flexible modes, partial_columns, format/fuzzy/jsonpaths, target/source combinations, omitted/direct/prefixed defaults, paused/single-table restrictions, and manual/auto resume were checked. Remaining Kinesis or pre-existing scheduler/descriptor behaviors are already threaded or outside this PR delta.
- Tests and results: Added parser, unit, replay, and regression coverage and expected output are consistent with the exercised paths, but there is no full RoutineLoadJob image round trip for a target/property switch and no delayed-journal metadata-lock test.
- Observability and error handling: Existing job/error logs are sufficient for these paths; no new metric is required. The cancellation catch and journal blocking are correctness issues rather than missing diagnostics.
- Persistence, failover, and mutation atomicity: Operation-log snapshot fields and expression/separator reconstruction are sound, and live/EditLog replay mostly agree. Checkpoint reconstruction does not, while journal durability is awaited in the wrong lock scope.
- Performance: Property staging and snapshot copies are bounded control-plane work; no hot-path CPU or memory issue was found. The unbounded metadata-lock hold is the material performance/availability concern.
- Other issues and user focus: The final full-diff/base-side/duplicate sweep found no additional PR-introduced issue. No additional user focus was provided.
Validation was static only: the review workflow prohibits builds/tests, and this checkout has no .worktree_initialized marker, thirdparty/installed, or protoc.
| modifyPropertiesInternal(command.getAnalyzedJobProperties(), dataSourceProperties, | ||
| preparedDataSourceProperties); | ||
| if (command.hasTargetTable()) { | ||
| tableId = command.getTargetTableId(); |
Contributor
There was a problem hiding this comment.
[P1] Make target/property alters survive checkpoint reload. EditLog replay restores this target and the new job properties, but RoutineLoadJob.gsonPostProcess() later reparses the unchanged CREATE origStmt, substitutes only the current table name, and validates the original properties against it. For example, a job created in fixed partial-update mode on MoW table A can switch to non-MoW B together with unique_key_update_mode=UPSERT, then be marked CANCELLED when an image reload validates the old fixed mode against B. Preserve the serialized current definition (or update the canonical statement) and add a full RoutineLoadJob image round-trip test.
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: Kafka routine load target-table ALTER revalidated the target while holding database and table metadata locks across the final in-memory mutation and edit-log write. The target has already been validated during command analysis, and each routine load task resolves the persisted table ID and replans under the table read lock before execution. Remove the duplicate mutation-time validation and metadata locks so ALTER commit is serialized only by the routine load job write lock. ### Release note None ### Check List (For Author) - Test: FE unit tests and FE build - `./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest` - `./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.load.routineload.RoutineLoadJobTest,org.apache.doris.load.routineload.RoutineLoadManagerTest,org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest` - `./build.sh --fe -j48` - Behavior changed: Yes. Target-table ALTER no longer repeats metadata validation during commit; task planning remains the authoritative runtime validation. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29647 ms |
Contributor
TPC-DS: Total hot run time: 177305 ms |
Contributor
ClickBench: Total hot run time: 25.12 s |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two blocking issues remain in the new target-table ALTER path.
Critical checkpoint conclusions:
- Goal and proof: The PR adds target switching and makes Kafka source/property transitions atomic and replayable. Happy-path, serialization, staging, and reset tests are present, but the target-only concurrency case and unauthorized negative path are incorrect, so the goal is not fully met.
- Scope and clarity: The implementation is focused on routine-load command analysis, Kafka mutation, persistence, parser support, and tests; no unrelated change or new server configuration was introduced.
- Concurrency and locking: The per-job lock serializes mutation, but target validation is performed before that lock and is not bound to the job definition, causing the first inline issue. Kafka/MetaService I/O is outside catalog/database/table metadata locks; no additional lock-order deadlock was substantiated.
- Lifecycle: PAUSE, explicit and automatic RESUME, STOP/CANCEL, task commit, replay, image reload, and every Cloud reset crash boundary were traced. The pending-reset protocol is retry-safe and idempotent; adjacent replay/image defects are already covered by existing threads.
- Configuration and conditions: Direct and
property.Kafka defaults, omission versus replacement, datetime origins, all offset forms, and pinned/dynamic partitions were checked. Remaining distinct source-path concerns are either base-side behavior or existing review threads. - Compatibility and parallel paths: Legacy Alter records default the new target/descriptor fields safely, and no new FE-BE field is introduced. Kafka and Kinesis parallel paths were checked; Kinesis issues found are pre-existing or already threaded.
- Error handling and security: Target type/temporary-table errors are emitted before authorization, creating the second inline issue. Other changed error paths preserve actionable job/request context.
- Tests and results: Changed unit tests and the ordered regression output are internally consistent. Missing deterministic concurrent-ALTER and unauthorized existing-versus-missing target tests correspond directly to the two findings. Tests were not executed because the review contract prohibits builds and test runs.
- Observability: Existing logs cover Kafka and MetaService failures with sufficient context; no new metric is required for this control path.
- Persistence, failover, and data correctness: Kafka live application and EditLog replay carry the source/job delta, target ID, and descriptor consistently; Cloud reset retry ordering is sound. Metadata mutation is still not atomic with target validation in the concurrency case reported inline.
- Performance and protocol: No new per-record hot-path work, material FE-BE protocol change, or memory-safety concern was introduced. External calls are confined to ALTER/RESUME control paths under the per-job lock.
- User focus: No additional user-provided review focus was specified.
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: ALTER ROUTINE LOAD target-table validation resolved target metadata before authorizing the current job and requested target. A limited-RBAC user could distinguish missing, non-OLAP, or temporary target tables through different validation errors. Authorize the current job first and check the target LOAD privilege by name before resolving database or table metadata. ### Release note Unauthorized ALTER ROUTINE LOAD target-table requests no longer reveal target table metadata. ### Check List (For Author) - Test: FE unit tests added but not run per user request - Behavior changed: Yes. Unauthorized requests now fail before target metadata resolution. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29470 ms |
Contributor
TPC-DS: Total hot run time: 177492 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review outcome: request changes. The review is complete after two full rounds; the normal core, normal surface, and risk-focused reviewers all converged on the same final ledger and returned NO_NEW_VALUABLE_FINDINGS.
Three new P1 findings are attached inline:
- The implementation is Kafka-only although the PR contract advertises Kafka/Kinesis target switching.
- A topic-only switch on a pinned Kafka job clears progress but does not re-seed the retained partitions, so resume builds tasks with null offsets.
- Flexible-update validation ignores a
COLUMNSdescriptor staged by the same ALTER and persists a definition CREATE explicitly rejects.
Critical checkpoint conclusions:
- Goal, scope, and tests: the 16 changed files are focused on FE routine-load ALTER syntax/state plus FE and regression coverage. The Kafka happy path is covered, but the advertised Kinesis path is absent, and coverage is missing for pinned topic-only resume and combined flexible-mode/
COLUMNSALTERs. - Correctness, concurrency, and parallel paths: job-lock publication and stale-task transaction fencing were traced. Existing target/mode, target/descriptor, mutable-table-ID, rename-authorization, and caller-context races were not duplicated. The new staged-definition and pinned-partition failures remain distinct.
- Lifecycle, persistence, and compatibility: live apply, EditLog replay, checkpoint reconstruction, Cloud reset retry, RPC partial failure, crash/failover, resume, and auto-resume were reviewed. Optional new log fields are backward-compatible and the deferred Cloud reset converges; existing checkpoint and follower-local replay defects remain covered by prior threads. The parser change appears intentional, but the Kinesis release contract does not match implementation.
- Security: authentication/authorization ordering was checked against the repository threat model. The current head addresses metadata lookup before target authorization; existing rename and mutable-table-ID privilege threads remain the applicable security findings. No additional distinct security issue was found.
- Configuration, protocol/data writes, performance, and observability: no new configuration items, FE/BE protocol variables, storage schema changes, or data-write paths were added. Kafka/MetaService work is staged outside metadata locks in the current head; no additional substantiated performance or observability issue was found.
- Validation status: no local build or test was run because the review contract forbids it. Live CI shows FE UT, compile, P0, external, nonconcurrent, cloud P0, and vault P0 passing;
check_coverage_feis failing. - Existing discussion and focus: live inline threads were re-read immediately before submission and these comments do not duplicate them. No additional user focus was supplied.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64878 Problem Summary: Keep ALTER ROUTINE LOAD target-table switching composable with existing job and Kafka/Kinesis data-source properties while removing unrelated Kafka ALTER atomicity, Cloud progress staging, partial-update compatibility, and RoutineLoadDesc persistence changes from this PR. ### Release note ALTER ROUTINE LOAD supports SET TARGET TABLE together with supported job and Kafka/Kinesis data-source properties. ### Check List (For Author) - Test: - Unit Test: Focused Routine Load FE UT, 34 tests passed - Build: ./build.sh --fe -j48 - Regression test: Added Kafka end-to-end coverage; not run locally because an isolated FE/BE/Kafka cluster was not started - Behavior changed: Yes. Paused single-table Kafka and Kinesis Routine Load jobs can switch target tables while preserving progress. - Does this need documentation: Yes. apache/doris-website#3988
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29187 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29668 ms |
Contributor
TPC-DS: Total hot run time: 177835 ms |
Contributor
ClickBench: Total hot run time: 25.06 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for four distinct correctness issues on the current head: the explicit FROM source type is discarded, target compatibility is checked against the old rather than effective job properties, and rejected combined ALTERs can leave unjournaled live mutations in both Kafka and Kinesis jobs.
Checkpoint conclusions:
- Goal and scope: the target-table switch is otherwise focused and the target-only Kafka/Kinesis plumbing, progress preservation, legacy-log zero default, and replay assignment are present. The four issues below prevent the advertised combined transition from being safe.
- Concurrency, lifecycle, and atomicity: paused-job locking and task clearing were traced through resume and transaction callbacks. Existing threads already cover mutable-target races; the two new source-specific findings below are distinct non-concurrent validation-to-commit failures.
- Validation and compatibility: TARGET remains non-reserved, ordinary string decoding is preserved, and target OLAP/non-temporary/single-table checks are present. However, the parsed source type is not preserved and same-statement effective update properties are omitted from target planning.
- Parallel paths and persistence: Kafka and Kinesis both assign and replay the new target ID, but both also mutate live source state before a throwable membership check and before the target/EditLog commit point, so a failed command diverges from durable state. Existing replay, image, Cloud, and journal-lock concerns were deduplicated against live threads.
- Tests and results: the changed tests cover matching parser forms, happy-path target switches, progress preservation, and legacy-log reading, but not these four negative cases. No local build or tests were run, as required by the review task. Live FE UT, compile, formatter, license, P0/nonconcurrent/external regression, performance, and vault P0 checks pass; cloud_p0 and check_coverage_fe are red, and their TeamCity details require authentication.
- Performance and observability: target validation adds planning only on explicit ALTER, not a scheduling hot path, and existing ALTER errors, job logs, SHOW state, and journal metrics remain available. No additional issue was found here.
- User focus: no extra focus points were supplied.
All three independent reviewers reread this exact four-comment set and returned NO_NEW_VALUABLE_FINDINGS on round 2. The live head and inline-thread set were refreshed immediately before submission, and no unresolved or duplicate candidate remains.
| } | ||
|
|
||
| return new AlterRoutineLoadCommand(labelNameInfo, loadPropertyMap, properties, dataSourceMapProperties); | ||
| return new AlterRoutineLoadCommand(labelNameInfo, targetTableName, |
Contributor
There was a problem hiding this comment.
[P1] Preserve and validate the explicit FROM type. The grammar records ctx.type, but this builder drops it; checkDataSourceProperties() then creates the source-property object from the job's existing type, so RoutineLoadManager's later comparison is always the job type against itself. For example, a Kafka job accepts SET TARGET TABLE = "B" FROM KINESIS("property.client.id"="x") as KafkaDataSourceProperties because property.* is legal there, despite the explicit KINESIS token. Carry the parsed type into the command and reject unknown or non-matching types before constructing the property object, with cross-type negative tests.
| checkDataSourceProperties(); | ||
| checkPartialUpdate(); | ||
| if (hasTargetTable()) { | ||
| validateTargetTable(ctx, job); |
Contributor
There was a problem hiding this comment.
[P1] Plan the target with the statement's effective job properties. This call validates B using the job's current update mode because analyzedJobProperties is never overlaid. A paused UPSERT job on MoW table A can therefore switch to non-MoW B while setting unique_key_update_mode=UPDATE_FIXED_COLUMNS: B is planned as UPSERT and accepted, both job implementations install the fixed-update mode and then B's ID, and the first resumed task fails with Only unique key merge on write support partial update. Build one staged task definition containing the target and analyzed property delta, validate that definition, and add this non-concurrent combined case.
| @@ -782,9 +782,12 @@ public void modifyProperties(AlterRoutineLoadCommand command) throws UserExcepti | |||
| } | |||
|
|
|||
| modifyPropertiesInternal(jobProperties, dataSourceProperties); | |||
Contributor
There was a problem hiding this comment.
[P1] Keep a failed combined ALTER side-effect-free. modifyPropertiesInternal() runs before the target assignment and journal, and it publishes custom Kafka properties before checkPartitions(). On a job consuming partition 0, SET TARGET TABLE = "B" FROM KAFKA("property.client.id"="new", "kafka_partitions"="999", "kafka_offsets"="0") installs client.id, then throws because partition 999 is absent; the command reports failure with the old target and no EditLog record, but the live job has changed until failover/restart. Validate immutable staged copies first, then publish the complete target/job/source delta atomically, and assert every field is unchanged on failure.
| @@ -687,9 +687,12 @@ public void modifyProperties(AlterRoutineLoadCommand command) throws UserExcepti | |||
| } | |||
|
|
|||
| modifyPropertiesInternal(jobProperties, dataSourceProperties); | |||
Contributor
There was a problem hiding this comment.
[P1] Apply the same failure atomicity to Kinesis. modifyPropertiesInternal() merges custom properties and replaces customKinesisShards before checkShards(), while the target assignment and EditLog call happen only afterward. A combined target ALTER that supplies property.client.timeout plus a nonexistent explicit shard therefore fails the membership check but leaves the live job's custom properties and pinned shard list changed, with the old target and no journal; resume and failover then observe different definitions. Stage source properties, shard lists/progress, common fields, and the target off-object, validate everything, and only then publish and journal the complete delta.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Routine Load target-table alteration was initially limited to the target-only syntax
ALTER ROUTINE LOAD FOR [db.]job ON table. It could not alter supported job or data-source properties in the same statement.This PR replaces that syntax with the explicit, composable form:
The implementation has the following behavior and constraints:
SET TARGET TABLEcan be combined with supported job properties and Kafka/Kinesis data-source properties.COLUMNS,WHERE, andPARTITION.LOADprivilege checks, and be compatible with the existing load description, distribution mode, and partial-update settings.FROMtype must match the existing job type; Kafka and Kinesis jobs cannot be converted into each other.kafka_topic, unlike StarRocks.Release note
ALTER ROUTINE LOADnow usesSET TARGET TABLE = "table"to switch the target table and supports combining the switch with supportedPROPERTIESandFROMdata-source properties in one statement.Check List (For Author)
Test
Behavior changed:
ON tabletoSET TARGET TABLE = "table", and supported job/source properties can now be altered in the same statement.Does this need documentation?
Check List (For Reviewer who merge this PR)