Uh oh!
There was an error while loading. Please reload this page.
[improve](streaming-job) support user-specified mysql server_id with per-reader assignment - #63490
Conversation
hello-stephen
commented
May 21, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
1315fab to
5696cc2CompareJNSimba
commented
May 21, 2026
/review |
JNSimba
commented
May 21, 2026
run buildall |
There was a problem hiding this comment.
Pull request overview
Adds support for a user-configurable MySQL CDC server_id (single value or range) for streaming jobs, and ensures parallel snapshot readers use distinct server_ids to avoid MySQL dump-thread collisions during snapshot backfill.
Changes:
- Introduces
server_idas a supported MySQL source property (and allows it through FE validation). - Adds
ConfigUtil.resolveServerIdRange()to parse/validate user input or deterministically derive a range fromjobIdsized tosnapshot_parallelism. - Updates
MySqlSourceReaderto build per-subtask MySQL source configs so each snapshot reader binds a uniqueserver_id, plus adds unit/regression coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/job_p0/streaming_job/cdc/test_streaming_mysql_job_server_id.groovy | Adds regression coverage for rejecting invalid server_id inputs and basic happy-path sync checks. |
| fs_brokers/cdc_client/src/test/java/org/apache/doris/cdcclient/utils/ConfigUtilTest.java | Adds unit tests for default-derived and user-specified server_id range resolution/validation. |
| fs_brokers/cdc_client/src/main/java/org/apache/doris/cdcclient/utils/ConfigUtil.java | Implements server_id range resolution, derivation, and cross-field validation vs snapshot_parallelism. |
| fs_brokers/cdc_client/src/main/java/org/apache/doris/cdcclient/source/reader/mysql/MySqlSourceReader.java | Applies per-reader server_id assignment by generating per-subtask configs from a resolved range. |
| fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/DataSourceConfigValidator.java | Allows server_id in streaming job source properties. |
| fe/fe-common/src/main/java/org/apache/doris/job/cdc/DataSourceConfigKeys.java | Adds the server_id config key and documents expected formats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I found one blocking issue in the server_id validation path. The PR's goal is clear and the reader-side per-subtask assignment is focused, but CREATE/ALTER validation does not currently enforce the new property's contract, so bad values can be accepted and persisted before the CDC client later fails.
Critical checkpoints: goal/test coverage: the feature is covered by unit/regression tests, but the new regression expectation for synchronous CREATE rejection is not satisfied by the current initReader path. Scope: the implementation is small and localized. Concurrency/lifecycle: no new shared mutable lifecycle issue found in the reader assignment path. Config items: server_id is a new source property and needs FE-side validation because it is user-facing and persisted. Compatibility/persistence: no storage-format or FE-BE protocol compatibility issue found. Parallel paths: CREATE/ALTER source validation should both reject malformed server_id consistently. Observability/performance/transactions: no additional issue found. User focus: no additional user-provided review focus.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
May 21, 2026
TPC-H: Total hot run time: 30804 ms |
hello-stephen
commented
May 21, 2026
TPC-DS: Total hot run time: 169540 ms |
hello-stephen
commented
May 21, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
May 21, 2026
FE Regression Coverage ReportIncrement line coverage |
JNSimba
commented
May 22, 2026
run buildall |
JNSimba
commented
May 22, 2026
/review |
There was a problem hiding this comment.
I completed the full PR review and did not find additional distinct inline issues beyond the already-open review context. I am still requesting changes because the existing FE validation thread remains a blocking correctness gap: server_id is accepted by DataSourceConfigValidator without source-type-specific syntax/range validation, so initial/snapshot CREATE JOB paths can persist invalid values before the CDC reader parses them. I am not adding a duplicate inline comment for that already-known thread.
Critical checkpoint conclusions:
- Goal/test: The PR aims to let MySQL CDC jobs use a user-provided or derived
server_idrange and adds unit/regression coverage. The reader-side range resolution and per-subtask config accomplish part of this, but CREATE-time validation is incomplete for initial/snapshot jobs. - Scope/focus: The implementation is mostly small and focused.
- Concurrency: The new per-reader IDs align with the existing snapshot polling concurrency model; I did not find a new distinct concurrency issue.
- Lifecycle/static init: No special static/global lifecycle issue found.
- Configuration:
server_idis a new source config; it needs FE validation at the persisted job boundary, especially because not all create paths initialize the reader. - Compatibility/persistence: Persisting invalid config is the main compatibility/correctness risk; no storage-format change found.
- Parallel paths: MySQL reader paths were updated; FE validation remains the uncovered parallel boundary.
- Conditions/error handling: Reader-side errors are explicit, but they occur too late for initial/snapshot create paths.
- Tests: Unit tests cover resolver behavior and regression tests cover happy paths plus latest-mode rejection, but they do not prove invalid initial/snapshot
server_idis rejected synchronously. - Observability/performance/transactions: No additional issue found.
User focus: No additional user-provided review focus was specified.
JNSimba
commented
May 22, 2026
/review |
There was a problem hiding this comment.
I found one blocking issue in the new server_id validation path.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds MySQL CDC server_id support and tests for create-time validation, but ALTER JOB coverage misses merged-property validation.
- Scope/clarity: the change is mostly focused, but the cross-field validator is called from an ALTER path with incomplete input.
- Concurrency/lifecycle: no new shared-state concurrency issue found in the reader changes; the issue is configuration lifecycle across paused ALTER and later task execution.
- Configuration compatibility: server_id is persisted job configuration; ALTER must validate the effective persisted config before accepting it.
- Parallel paths: CREATE validates the full source map, but ALTER validates only the source-property delta, so the paths are inconsistent.
- Tests: new CREATE/regression coverage does not cover ALTERing server_id on an existing job with snapshot_parallelism > 1.
- Focus points: no additional user-provided focus points were supplied.
Please fix the ALTER path to validate the merged source properties, or make server_id immutable when snapshot_parallelism is already fixed.
Uh oh!
There was an error while loading. Please reload this page.
JNSimba
commented
May 25, 2026
/review |
There was a problem hiding this comment.
Review conclusion: I did not find additional non-duplicate inline findings beyond the existing review threads, but the current head still needs changes before merge. The already-open threads around FE server_id validation/source-type handling and ALTER JOB effective-property validation remain the blocking items to resolve.
Critical checkpoints:
- Goal/test coverage: the PR aims to let MySQL CDC jobs configure safe server_id ranges and to derive distinct IDs for parallel snapshot readers. The cdc_client path now uses ranges and there are FE/cdc_client unit tests plus a MySQL regression test, but the existing review threads identify CREATE/ALTER validation gaps that mean the tests do not fully prove invalid persisted/effective configs are rejected at analysis time.
- Scope/focus: the implementation is mostly focused on server_id handling, with small changes in FE validation, TVF validation, cdc_client config generation, and tests.
- Concurrency: the runtime change affects parallel snapshot readers by assigning per-subtask server IDs. I did not find a new lock-order or shared-state concurrency issue in this pass.
- Lifecycle/static initialization: no new static initialization-order or lifecycle-management issue found.
- Configuration compatibility: server_id is a new source property and existing jobs without it continue to derive IDs, but the already-open validation comments need resolution for invalid or non-MySQL usage and ALTER merged-property handling.
- Parallel code paths: cdc_stream TVF and CREATE JOB paths were considered; no additional distinct path issue found beyond the existing validation threads.
- Error handling: no ignored Status-style issue applies in this Java path; user-facing validation errors are generally propagated as analysis failures, subject to the existing gaps.
- Persistence/transactions/data correctness: no new transaction or persistence issue found; the concern remains preventing invalid source configs from being persisted/effective.
- Observability/performance: no additional blocking issue found; added server_id logs are lightweight.
- User focus: no additional user-provided review focus was specified.
Please address the existing inline threads before this PR is merged.
JNSimba
commented
May 26, 2026
run buildall |
hello-stephen
commented
May 26, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
May 26, 2026
TPC-H: Total hot run time: 31215 ms |
JNSimba
commented
May 26, 2026
run buildall |
hello-stephen
commented
May 26, 2026
TPC-DS: Total hot run time: 172998 ms |
hello-stephen
commented
May 26, 2026
TPC-H: Total hot run time: 31835 ms |
hello-stephen
commented
May 26, 2026
TPC-DS: Total hot run time: 172440 ms |
…per-reader assignment
…n regression assertions
…ession and drop stale comment
a68a33e to
a31e7d7CompareJNSimba
commented
May 26, 2026
/review |
JNSimba
commented
May 26, 2026
run buildall |
hello-stephen
commented
May 26, 2026
TPC-H: Total hot run time: 31842 ms |
hello-stephen
commented
May 26, 2026
TPC-DS: Total hot run time: 170745 ms |
JNSimba
commented
May 26, 2026
run valut_p0 |
There was a problem hiding this comment.
I found one blocking correctness issue in the new server_id validation.
Critical checkpoint conclusions:
- Goal/test: The PR aims to support configurable MySQL CDC server_id and validate invalid ranges earlier. It covers initial snapshot paths, but the validation now rejects valid binlog-only configurations, and there is no test for offset=latest/earliest/specific offset with snapshot_parallelism > 1.
- Scope/clarity: The change is focused, but the shared validation/runtime resolver uses snapshot_parallelism without considering startup mode.
- Concurrency/lifecycle: The snapshot reader path uses per-subtask server IDs for parallel readers; no additional lock/lifecycle issue found in the reviewed change.
- Config/compatibility: server_id is a new source property; existing jobs without it still derive IDs. The reported issue is a behavior regression for valid non-snapshot jobs that set snapshot_parallelism and a single explicit server_id.
- Parallel paths: CREATE/TVF/runtime paths all share or mirror the same over-strict width requirement, so the problem affects analysis and CDC client execution.
- Tests: Added tests cover malformed values and initial-mode happy paths, but not binlog-only modes where only one server_id is needed.
- Observability/performance/transaction/persistence: No additional issue found for these checkpoints.
User focus: No additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
May 26, 2026
FE UT Coverage ReportIncrement line coverage |
JNSimba
commented
May 26, 2026
run vault_p0 |
hello-stephen
commented
May 26, 2026
FE Regression Coverage ReportIncrement line coverage |
JNSimba
commented
May 27, 2026
run vault_p0 |
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
#63404#63471#63480#63490#63514#63618 (#63812) Cherry-picked from: - #63079 [improve](streaming-job) async chunk splitting for cdc source job - #63404 [test](streaming-job) refine cdc data-type and boundary regression cases for mysql/pg - #63471 [regression-test](streaming-job) add cdc cases (composite/concurrent-dml/id-gap/decimal/datetime pk) and fix split-bound java.time deserialize - #63480 [fix](streaming-job) misc fixes for typo/log/validation/visibility - #63402 [fix](streaming-job) drop neighbour-table rows leaked by JDBC LIKE wildcards in JdbcPostgreSQLClient - #63514 [regression-test](streaming-job) add cdc operational cases for offset modes and pg slot lifecycle - #63618 [fix](streaming-job) fix postgres historical-date timestamp handling in cdc-client - #63490 [improve](streaming-job) support user-specified mysql server_id with per-reader assignment
…per-reader assignment (apache#63490) ## Summary - Add an optional `server_id` source property for MySQL CDC streaming jobs. Accepts a single value (e.g. `5400`) or a range (e.g. `5400-5408`). When unset, the value is derived from the jobId hash so existing jobs keep their current server_id when `snapshot_parallelism = 1`. - Fix a latent collision: when `snapshot_parallelism > 1` and source-side DML happens during snapshot, all parallel `SnapshotSplitReader` instances previously shared the same server_id and their backfill BinaryLogClient connections kicked each other out of MySQL's dump-thread slot, dropping binlog events between low and high watermark. Each subtask now gets a distinct server_id from the resolved range; the single binlog reader uses the range start. - Cross-field check: reject `server_id` range width smaller than `snapshot_parallelism` at job startup with a clear fix-it suggestion.
Summary
server_idsource property for MySQL CDC streaming jobs. Accepts a single value (e.g.5400) or a range (e.g.5400-5408). When unset, the value is derived from the jobId hash so existing jobs keep their current server_id whensnapshot_parallelism = 1.snapshot_parallelism > 1and source-side DML happens during snapshot, all parallelSnapshotSplitReaderinstances previously shared the same server_id and their backfill BinaryLogClient connections kicked each other out of MySQL's dump-thread slot, dropping binlog events between low and high watermark. Each subtask now gets a distinct server_id from the resolved range; the single binlog reader uses the range start.server_idrange width smaller thansnapshot_parallelismat job startup with a clear fix-it suggestion.Test plan
ConfigUtilTest: 15 cases covering default-derive (Integer.MAX_VALUE hash clamp, hash=0 bump), user single value / range, malformed input, blank input, range-vs-parallelism width check, non-positive parallelism.test_streaming_mysql_job_server_id.groovy: 4 rejection cases (format / zero / backward / width) underoffset=latestfor synchronous CREATE JOB feedback, plus 3 happy-path cases verifying snapshot data syncs under single value / range / default-derive.