davidzollo commented on PR #11206: URL: https://github.com/apache/seatunnel/pull/11206#issuecomment-5343030346
# What Problem Does This PR Solve? **User pain point:** a MySQL CDC job configured with `table-pattern`/`database-pattern` cannot pick up a table created *after* the job starts. Debezium matches the new table's row events against the pattern at the source level, but SeaTunnel's reader never registers a schema/converter for it, so the rows are silently dropped, and the JDBC sink has no mechanism to create the corresponding target table on the fly. **Fix approach:** the PR adds `scan.newly-added-table.enabled` (formalizes the pre-existing restore-time re-snapshot behavior, default `true`, no behavior change) and `scan.binlog.newly-added-table.enabled` (new, default `false`) — the latter is meant to parse the `CREATE TABLE` entry out of Debezium's `tableChanges` while the reader is already streaming binlog, build a `CatalogTable`, and emit a `CreateTableEvent`. On the sink side it adds `SupportMultiTableSinkWriter#createSinkWriter` and a `RuntimeSinkWriterFactory` hook so `MultiTableSinkWriter` can spin up a brand-new per-table JDBC writer at runtime and physically create the target table via `catalog.createTable(...)`. **One-sentence summary:** the design direction is sound and the SeaTunnel-side wiring I re-traced this round is structurally correct, but I pulled the raw job log from this exact head's own CI run and found hard, direct evidence that the headline capability has never once completed end-to-end in any of the 10 executions of its own e2e test — this is a stronger finding than what earlier rounds on this PR reported, and it corrects an over-optimistic claim from my own previous round. --- ## Important correction to my own previous round (2026-08-18) My last review on this exact head (`60d4e4155519`, unchanged since) stated: *"the normal streaming path does reach `handleTableChangeStruct`... the mechanism is now proven to reach the sink and materialize the table on every single one of 10 CI executions I inspected."* I went back to the actual CI job log for this same run this round (`mysql-cdc-connector-it (8, ubuntu-latest)`, run `32043314641`, job `95426735095`, https://github.com/DanielLeens/seatunnel/actions/runs/32043314641/job/95426735095) and that claim does not hold up: - `grep -c "Registered newly added CDC table"` (the exact success log at `SeaTunnelRowDebeziumDeserializeSchema.java:152`, emitted only when `handleTableChangeStruct` actually registers a new table) against the full 233,089-line job log → **0 matches**, across all 10 test executions in the run. - `grep -c "Registered runtime sink writer for newly created table"` (the sink-side success log at `MultiTableSinkWriter.java:751-753`) → **0 matches**. - `grep -c "CreateTableEvent"` anywhere in the log (class name, in any stack trace or log line) → **0 matches**. - Every one of the 10 executions of `testMysqlCdcByWildcardsConfigWithNewlyAddedTable` (5 on `Mysql8_4CDCIT`, 5 on `MysqlCDCIT`, across engines) fails identically: `org.awaitility.core.ConditionTimeoutException: ... newly added wildcard sink table not readable yet: java.sql.SQLSyntaxErrorException: Table 'sink.source_payments' doesn't exist within 2 minutes`, with the whole test consistently costing 152-160s (i.e. exactly the 120s `awaitility` budget plus fixed setup overhead — not the tens-of-minutes-then-succeeds pattern I described last round). - I did independently confirm the DML row for the new table *is* delivered to the reader: at `2026-08-17T20:43:48Z` the log shows a `READER_MESSAGE_DELAYED` event for `ConnectRecord{topic='mysql_binlog_source.source.payments', ... op=c, ...}` — Debezium is capturing binlog rows for the new table. But that is upstream of `deserialize()`/`shouldEmit()` filtering, not proof of successful registration, and the actual registration log never appears anywhere afterward. I'm flagging this explicitly rather than quietly reissuing a corrected number, because it changes the merge-blocking finding from "works but too slow to demonstrate in CI" to "has never been observed to complete, in any execution, of its own test." I don't have a fully pinned root cause (see 1.1 below for what I did trace), but the empirical evidence is unambiguous, and it reconfirms — not refutes — the original concern raised earlier in this PR's history that the source-side registration ("Registered newly added CDC table") never fires on the real streaming path. --- # 1. Code Change Review ## 1.1 Core Logic Analysis **Files re-traced this round:** `SeaTunnelRowDebeziumDeserializeSchema.java`, `AbstractDebeziumDeserializationSchema.java`, `MySqlIncrementalSource.java`, `MySqlSourceConfigFactory.java`, `IncrementalSourceStreamFetcher.java` (source side, unmodified by this PR but on the critical path), `MultiTableSinkWriter.java`, `MultiTableSink.java` (sink side). **What I verified is correctly wired (source-level, not just CI-observed):** ```java // SeaTunnelRowDebeziumDeserializeSchema.java:126-154 protected void handleTableChangeStruct(Struct tableChangeStruct) { if (!scanBinlogNewlyAddedTableEnabled || tableChangeCatalogTableConverter == null) { return; } ... tables.add(catalogTable); pendingCreateTables.add(catalogTable); tableRowConverters = createTableRowConverters(...); log.info("Registered newly added CDC table {}", catalogTable.getTablePath()); } ``` - `MySqlIncrementalSource.java:284-292` wires both `scanBinlogNewlyAddedTableEnabled` (from `SCAN_BINLOG_NEWLY_ADDED_TABLE_ENABLED`) and `tableChangeCatalogTableConverter` (a real `MySqlCatalogTableUtils.toCatalogTable(...)` lambda, not null) into the builder, so the early-return guard should not trip when the option is on. - `MySqlSourceConfigFactory.java:105-107` sets Debezium's `include.schema.changes=true` whenever `scanBinlogNewlyAddedTableEnabled` is set, which is what's supposed to make Debezium emit `TABLE_CHANGES`-bearing history records for DDL encountered on the binlog. - `AbstractDebeziumDeserializationSchema.java:65-77`'s `deserialize()` unconditionally calls `handleTableChangeStruct` for every record where `isSchemaChangeEvent(record)` is true, and `SeaTunnelRowDebeziumDeserializeSchema.deserialize()` calls `super.deserialize(...)` first (`:169`) — so there's no dead branch *inside this class*. - `IncrementalSourceStreamFetcher.shouldEmit(...)` (`:265-294`, unmodified by this PR) returns `true` unconditionally for any record that is *not* a data-change record (`:293-294`), i.e. schema-change/DDL records are not filtered out by table-membership here. So the DDL-suppression, if it exists, is not in this filter. - The e2e conf (`mysqlcdc_wildcards_with_newly_added_table.conf`) sets both `schema-changes.enabled=true` and `scan.binlog.newly-added-table.enabled=true`, and `table-pattern="source.*\\..*"` does match `source.payments`. **What actually happens in CI (the part that matters):** despite all of the above being wired correctly at the layer I can read statically, the success log at the end of `handleTableChangeStruct` never fires. The most likely remaining candidates are further upstream than this PR's diff — in how the custom `EmbeddedDatabaseHistory` (used at `MySqlSourceConfigFactory.java:94`, not part of this diff) interacts with Debezium's actual DDL-to-history-record emission for a table `CREATE`d after the connector has already left the snapshot phase, or in the per-table watermark bookkeeping in `IncrementalSourceStreamFetcher`'s data-record path swallowing the record before a schema-change record is ever produced for it. I was not able to fully pin this down through static reading in the time available; I flag it as the concrete next investigation step rather than guessing further. **Key findings:** 1. The SeaTunnel-side wiring for this feature (config plumbing, builder wiring, event dispatch, filter pass-through) is structurally sound wherever I could verify it directly. 2. Despite that, the feature's success path has literally never been observed to fire in any of the 10 executions of its own e2e test in the current CI run for this exact head. 3. This is not a defensive fix or workaround — it is meant to be the PR's headline capability — and it is currently inert on the one path that's supposed to prove it: the job's own binlog-streaming e2e test. 4. Because the failure is 10/10 reproducible with an identical error and identical timing envelope, this reads as a deterministic gap, not CI flakiness or a slow-but-working mechanism. **Runtime path (intended, per source; the point marked ✗ is where CI evidence says it actually stops)** ```text Binlog streaming phase (scan.binlog.newly-added-table.enabled = true) -> Debezium/EmbeddedDatabaseHistory should deliver a schema-change record for "CREATE TABLE source.payments" -> IncrementalSourceStreamFetcher.shouldEmit(record) -> true (non-data-change records pass through) [:293-294] -> AbstractDebeziumDeserializationSchema.deserialize() [:65-77] -> handleTableChangeStruct(tableChangeStruct) [SeaTunnelRowDebeziumDeserializeSchema.java:127-154] -> log.info("Registered newly added CDC table {}", ...) ✗ NEVER OBSERVED IN CI (0/233089 log lines) -> emitPendingCreateTableEvents(...) -> CreateTableEvent ✗ never observed (0 matches for "CreateTableEvent" anywhere in the log) Sink side (MultiTableSinkWriter.registerNewlyCreatedTable) ✗ never reached (0 matches for its success log) Test assertion -> queryNewlyAddedWildcardSinkTable() polls "sink.source_payments" for 120s -> ConditionTimeoutException every time: "Table 'sink.source_payments' doesn't exist" (10/10 executions, ~153-160s each) ``` ## 1.2 Compatibility Impact **Partially incompatible.** - `scan.binlog.newly-added-table.enabled` defaults to `false` — no behavior change for existing jobs that don't opt in. - `scan.newly-added-table.enabled` defaults to `true` and formalizes the pre-existing restore-time re-snapshot behavior — no behavior change. - Checkpoint/state structure is unchanged; old checkpoints remain structurally loadable. - One real, always-on compatibility change: `MultiTableSink.registerAggregatedFlush(...)` is now invoked unconditionally from both `createWriter` (`MultiTableSink.java:174`) and `restoreWriter` (`:245`) for *every* `MultiTableSink`, not just jobs using this new feature — see Issue 5 below. ## 1.3 Performance / Side-Effect Analysis - `MultiTableSinkWriter.registerNewlyCreatedTable` (`:683-757`) holds `synchronized (runnable.get(i))` (`:691`) across the entire per-queue writer creation, including `runtimeSinkWriterFactory.create(catalogTable, context)` (`:713`), which for JDBC ends in a live `catalog.createTable(...)` DDL round-trip in `AbstractJdbcSinkWriter`. This is a lock held across blocking I/O; under `multi_table_sink_replica > 1` or high per-queue contention this serializes registration and is a real, if currently unobserved-in-passing-CI, latency/contention risk. I re-confirmed this pattern is unchanged this round. - `tableRowConverters` is rebuilt for the *entire* `tables` list every time a single new table is registered (`SeaTunnelRowDebeziumDeserializeSchema.java:146-151`) — O(all known tables) per discovery event, not O(1). - No upper bound on the number of runtime-registered tables; a broad `table-pattern` combined with `scan.binlog.newly-added-table.enabled=true` can accumulate sink writers without limit. ## 1.4 Error Handling and Logging **Issue 1: The feature's own success path has never been observed to fire in CI; its e2e test fails deterministically, not intermittently** - **Location:** `SeaTunnelRowDebeziumDeserializeSchema.java:152` (never-logged success line); `MultiTableSinkWriter.java:751-753` (never-logged sink-side success line); test at `seatunnel-e2e/seatunnel-connector-v2-e2e/connector-cdc-mysql-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/AbstractMysqlCDCITBase.java:845-881` - **Problem description:** In the current head's own CI run (`mysql-cdc-connector-it (8, ubuntu-latest)`, https://github.com/DanielLeens/seatunnel/actions/runs/32043314641/job/95426735095), `testMysqlCdcByWildcardsConfigWithNewlyAddedTable` fails all 10/10 times it runs (5 container variants x 2 engine test classes) with the identical error `Table 'sink.source_payments' doesn't exist within 2 minutes`, each execution costing ~153-160s (bounded by the 120s `await()` plus fixed overhead — not a slow-but-eventually-successful pattern). Neither of the two success log lines that would prove the mechanism worked appear anywhere in the 233k-line job log, and the string `CreateTableEvent` does not appear anywhere either (not even in a stack trace). I did confirm Debezium *is* delivering DML for the new table to the reader (a `READER_MESSAGE_DELAYED` event for `source.payments` row `id=2001` at `20:43:48Z`), so the gap is specifically in the DDL/schema-change registration path, not in bi nlog delivery generally. - **Potential risk:** the PR's headline capability — binlog-driven newly-added-table discovery — does not currently work at all on the one path built to demonstrate it. Merging this as-is ships a documented, opt-in feature (`docs/en/connectors/source/MySQL-CDC.md`'s new "Newly Added Tables" section) that a user who enables it will find silently does nothing: rows for the new table are captured by Debezium but permanently dropped (`SeaTunnelRowDebeziumDeserializeSchema.java:328-331`, at `log.debug`, so effectively invisible operationally) because the table's converter and the sink's writer are never registered. - **Best improvement:** before this can be considered for merge, get `testMysqlCdcByWildcardsConfigWithNewlyAddedTable` passing against a real run and confirm the "Registered newly added CDC table" log actually appears. The most productive place to continue tracing is upstream of this PR's diff: how `EmbeddedDatabaseHistory` (`MySqlSourceConfigFactory.java:94`) surfaces DDL processed after the connector has already left the snapshot phase, and whether a schema-change `SourceRecord` is actually produced/queued for `CREATE TABLE source.payments` at all before it ever reaches `AbstractDebeziumDeserializationSchema.deserialize()`. - **Severity:** High (Blocker) - **Raised by another reviewer:** No — this specific "never fires in CI, not just slow" framing is new evidence from this round; it corrects a more optimistic characterization from my own 2026-08-18 comment on this same PR, which I'm superseding here. **Issue 2 — A binlog-registered table is not checkpointed and would be silently forgotten after any restart** - **Location:** `SeaTunnelRowDebeziumDeserializeSchema.java:121-124`,`:144-152` (`tables`/`tableRowConverters` are plain mutable fields, no checkpoint-state hook); `MultiTableSink.java:199-230` (`restoreWriter` iterates only `sinks.keySet()`, the config-time table set — re-verified this round by direct read) - **Problem description:** `restoreWriter` never sees a table registered purely at runtime via `registerNewlyCreatedTable`, because that registration only mutates in-memory maps on the live `MultiTableSinkWriter` instance and is never folded into `MultiTableState`. - **Potential risk:** would be silent, permanent data loss for exactly the tables this feature exists to capture, on the next restart — a routine event for a long-running CDC job. (This is currently unobservable in practice only because Issue 1 means the table is never successfully registered in the first place; it becomes a live risk the moment Issue 1 is fixed.) - **Best improvement:** persist the set of runtime-registered `CatalogTable`s/`TablePath`s into checkpoint state so restore can re-run the equivalent of `registerNewlyCreatedTable` on recovery. - **Severity:** High - **Raised by another reviewer:** No. **Issue 3 — No capture-pattern check before registering a table discovered from binlog DDL** - **Location:** `SeaTunnelRowDebeziumDeserializeSchema.java:127-154` (re-verified: `handleTableChangeStruct` only checks `containsTable` for dedup, never re-checks the new table's path against `table.include.list`/`table-pattern`) - **Problem description:** every `TableChangeType.CREATE` observed is registered unconditionally, with no filter against the configured capture pattern. - **Potential risk:** directly contradicts this PR's own new documentation (`docs/en/connectors/source/MySQL-CDC.md`: "The new table must match the configured capture pattern") — the doc makes a scoping promise the code does not enforce. A job capturing `orders_.*` could start registering `audit_log` the moment it's created in the same database. - **Best improvement:** apply the same table-filter logic the connector already uses elsewhere inside `handleTableChangeStruct` before `tables.add(catalogTable)`. - **Severity:** High - **Raised by another reviewer:** No. **Issue 4 — `CreateTableEvent` bypasses `schemaChangeEventFilter`** - **Location:** `SeaTunnelRowDebeziumDeserializeSchema.java:194-196` (`emitPendingCreateTableEvents` returns `true`, short-circuiting before the filter) vs. `:213-215` (the filter every other schema-change event goes through) — re-verified by direct read this round. - **Problem description:** every other schema-change event is passed through `schemaChangeEventFilter.filter(...)` before being collected; `CreateTableEvent`s are not. - **Potential risk:** a user who excludes schema changes via `schema-changes.exclude` would still receive `CreateTableEvent`s for newly discovered tables — inconsistent with the rest of the same job's configuration. - **Best improvement:** route the emitted `CreateTableEvent` through `schemaChangeEventFilter` before `collector.collect(...)`, same as the rest of the method. - **Severity:** Medium-High - **Raised by another reviewer:** No. **Issue 5 — Aggregated flush is now registered unconditionally for every multi-table sink job, not just this feature's users** - **Location:** `MultiTableSink.java:174` (`createWriter`), `:245` (`restoreWriter`), both calling `registerAggregatedFlush(context, writer)` (`:291-293`) — re-verified by direct read this round. - **Problem description:** every `MultiTableSink`, including jobs that never touch `scan.binlog.newly-added-table.enabled`, now registers this new flush action on upgrade. - **Potential risk:** a runtime-behavior change for existing production multi-table-sink jobs outside the opt-in envelope the rest of this PR otherwise maintains. - **Best improvement:** gate `registerAggregatedFlush` behind whether `runtimeSinkWriterFactory` is non-null, rather than calling it unconditionally. - **Severity:** Medium - **Raised by another reviewer:** No. **Issue 6 — Single-table jobs bypass the unknown-table guard entirely** - **Location:** `SeaTunnelRowDebeziumDeserializeSchema.java:317-335`, the `else` branch at `:333-335` — re-verified by direct read this round. - **Problem description:** the `tables.size() > 1` guard that protects against forwarding rows for an unregistered table only applies to the multi-table branch; the single-table `else` branch unconditionally uses `DEFAULT_TABLE_NAME_KEY`. - **Potential risk:** low today (a single-table job has no meaningful newly-added-table scenario), but an inconsistency that would surface if this code is reused for a job that starts single-table and later gains tables. - **Best improvement:** apply the same `tableId`-keyed lookup in both branches, or document why the single-table path is exempt. - **Severity:** Medium - **Raised by another reviewer:** No. **Issue 7 — Duplicated builder call, unfixed across multiple review rounds** - **Location:** `MySqlIncrementalSource.java:282-283` — re-confirmed present, unchanged, this round. - **Problem description:** `.setSchemaChangeEventFilter(SchemaChangeEventFilter.fromConfig(config))` appears twice in a row, identically. - **Potential risk:** none functionally (idempotent), but a signal that small, easy review comments on this PR aren't being worked through. - **Best improvement:** delete the duplicate line. - **Severity:** Low - **Raised by another reviewer:** No. **Issue 8 — Silently no-ops in `table-names` mode, with no validation** - **Location:** `MySqlSourceConfigFactory.java:133-136` (`table.include.list` set from the fixed `tableList` when `table-names` is used, not a pattern). - **Problem description:** `scan.binlog.newly-added-table.enabled` is only meaningful when new tables can match a pattern; in `table-names` mode the include list is a fixed enumeration, so no table created after job start can ever match. There is no validation warning about this combination. - **Best improvement:** reject or warn in the factory's option validation when `scan.binlog.newly-added-table.enabled=true` is combined with `table-names`. - **Severity:** Medium - **Raised by another reviewer:** No. No sensitive data is logged anywhere in the new code. --- # 2. Code Quality Assessment ## 2.1 Coding Standards Javadoc is present on new public/protected methods (`handleTableChangeStruct`, `registerNewlyCreatedTable`, `registerAggregatedFlush`, `isScanNewlyAddedTableEnabledOnRestore`, the new `Option`s), no wildcard imports, no `System.out.println`, ASF headers present on new files. The `containsTable` and `emitPendingCreateTableEvents` Javadocs are clear about *why*, not just *what*. Issue 7's duplicated line is the one concrete, trivial miss. ## 2.2 Test Coverage and Test Stability **Stability rating: High risk.** - `testMysqlCdcByWildcardsConfigWithNewlyAddedTable` fails deterministically (10/10) at the current head — see Issue 1. Per the review protocol's rule, a test that cannot pass in CI must be rated High risk regardless of suspected root cause. - The `Awaitility` polling itself is well-structured (`untilAsserted`, condition-based) and the most recent commit (`60d4e4155519`, "[Test][E2E] Retry newly-added wildcard sink polls...") correctly fixes a real bug where `query()`'s `SQLException`-wrapped-in-`RuntimeException` was aborting the poll on its first legitimate "table not created yet" hit instead of retrying — I confirmed this fix is syntactically correct and does not itself explain Issue 1 (the underlying table genuinely never gets created within the test's lifetime, retried or not). - No new test restarts a job across the table-addition window (would exercise Issue 2), no negative test asserts a non-matching table is *not* registered (would catch Issue 3), no test for `schema-changes.exclude` interaction with `CreateTableEvent` (would catch Issue 4), no test for the `table-names` mode combination (would catch Issue 8). Only one new e2e conf (`mysqlcdc_wildcards_with_newly_added_table.conf`) exists, and it doesn't currently pass. - No hard-coded ports, no shared static state, no order-dependent assertions observed in the new test code itself. ## 2.3 Documentation Updates `docs/en/connectors/source/MySQL-CDC.md` and `docs/zh/connectors/source/MySQL-CDC.md` were both updated (8 lines each) with a new options table rows and a "Newly Added Tables" subsection. Problems: - The doc's claim "The new table must match the configured capture pattern" is not enforced by the code (Issue 3). - No mention that a runtime-discovered table is lost on restart (Issue 2). - No mention of the `table-names` mode restriction (Issue 8). - Most importantly right now: the documented behavior ("registers table metadata from the CREATE TABLE schema record and then reads following DML records for that table") does not match what the current CI run demonstrates (Issue 1). --- # 3. Architectural Soundness ## 3.1 Elegance of the Solution The binlog-ordering insight (MySQL cannot emit a row event for a table before its own `CREATE TABLE`, so there's no missed-event race on discovery ordering itself) is sound, and the base-class hook design (`handleTableChangeStruct` as an overridable no-op) is clean. But given Issue 1, this is currently best classified as an **incomplete implementation of a precise-fix design** — the architecture is right, the mechanism as implemented and shipped in this diff does not demonstrably work. ## 3.2 Maintainability `handleTableChangeStruct`, `registerNewlyCreatedTable`, and `restoreWriter` each touch overlapping "what tables does this job know about" state through three different, uncoordinated data structures (`tables`/`tableRowConverters` on the source; `sinks`/`sinkWritersWithIndex` on the sink), none backed by checkpoint state for the runtime-added case. That's the root of Issue 2 and would benefit from a single source of truth before this is extended further. ## 3.3 Extensibility The `RuntimeSinkWriterFactory`/`SupportMultiTableSinkWriter#createSinkWriter` pattern is a reasonable, reusable extension point for other sink connectors to support runtime table creation in the future, independent of whether the source-side mechanism above is fixed. ## 3.4 Historical-Version Compatibility No checkpoint/state schema change, no serialization format change for existing checkpoints. Issue 5 (unconditional aggregated-flush registration) is the one behavior change affecting existing jobs on upgrade. --- # 4. Issue Summary | # | Issue | Location | Severity | |---|---|---|---| | 1 | Feature's own e2e test fails deterministically (10/10) at current head; success logs on both source and sink side never appear in CI | `AbstractMysqlCDCITBase.java:845-881`; `SeaTunnelRowDebeziumDeserializeSchema.java:152`; `MultiTableSinkWriter.java:751-753` | High (Blocker) | | 2 | Binlog-registered table not checkpointed; would be silently forgotten after restart | `SeaTunnelRowDebeziumDeserializeSchema.java:121-152`; `MultiTableSink.java:199-230` | High | | 3 | No capture-pattern check before registering a DDL-discovered table; contradicts PR's own docs | `SeaTunnelRowDebeziumDeserializeSchema.java:127-154` | High | | 4 | `CreateTableEvent` bypasses `schemaChangeEventFilter` | `SeaTunnelRowDebeziumDeserializeSchema.java:192-215` | Medium-High | | 5 | Aggregated flush registered unconditionally for all multi-table sink jobs on upgrade | `MultiTableSink.java:174`, `:245`, `:291-293` | Medium | | 6 | Single-table jobs bypass unknown-table guard | `SeaTunnelRowDebeziumDeserializeSchema.java:317-335` | Medium | | 7 | Duplicated `.setSchemaChangeEventFilter(...)` call, unfixed across rounds | `MySqlIncrementalSource.java:282-283` | Low | | 8 | Silently no-ops in `table-names` mode, no validation or doc note | `MySqlSourceConfigFactory.java:133-136` | Medium | --- # 5. Merge Recommendation ### Conclusion: Not recommended for merge **1. Blockers — must be fixed** - **Issue 1** — the feature's own e2e test does not pass, and there is no CI evidence anywhere in the current run that the source-side registration (`"Registered newly added CDC table"`) or sink-side registration ever succeeds. This needs to be root-caused (see the "next investigation step" note in Issue 1) and demonstrated passing before this can move forward — not worked around with a longer timeout, since the current run shows no sign of eventual success either. - **Issue 2** — no checkpoint persistence for runtime-registered tables; becomes a live silent-data-loss risk the moment Issue 1 is fixed. - **Issue 3** — no capture-pattern filter on binlog-discovered tables, contradicting the PR's own documentation. **2. Recommended fixes — non-blocking** - Issue 4 (schema-change filter bypass), Issue 5 (unconditional aggregated-flush registration), Issue 6 (single-table guard inconsistency), Issue 8 (`table-names` no-op with no validation) — real but narrower in blast radius than the blockers above. - Issue 7 (duplicated line) — trivial, easy to fold into the same pass. **Overall assessment** The design direction remains reasonable and the parts of the implementation I could verify statically (config plumbing, event dispatch, filter pass-through, the sink-side extension point) are wired correctly. But this round's contribution is a correction, not a reconfirmation: going back to the actual CI log for the exact current head rather than trusting my own prior notes, I found zero evidence — not "slow" evidence, zero evidence — that the mechanism this PR is meant to add has ever completed successfully. That's a more fundamental gap than "needs a longer test timeout," and it should be root-caused (I'd start by checking whether `EmbeddedDatabaseHistory` actually produces a schema-change `SourceRecord` for a table `CREATE`d after the connector leaves the snapshot phase — that's the one link in the chain I could not verify by reading source alone) and demonstrated with a genuinely passing e2e run before the durability (Issue 2) and scoping (Issue 3) gaps are worth investi ng further review cycles in. Because GitHub does not allow approving one's own pull request, this review is submitted as a plain comment rather than a formal review. It carries no approval weight, and merge still requires review and approval from another committer. The PR remains in **draft** state; GitHub currently reports `mergeStateStatus=DIRTY`/`mergeable=CONFLICTING` against `dev`, which will need to be resolved independently of the findings above. Separately from this feature's own test, this run also shows failures in `Dead links` (pre-existing/unrelated — it's reporting 404s for files that don't exist in `dev` itself, e.g. `docs/en/developer/new-license.md`, `plugin-mapping.properties`, unrelated to this PR's diff), `unit-test (windows-latest)`, `Dependency licenses`, `engine-k8s-it`, `transform-v2-it-part-1`, and `all-connectors-it-5`; I did not have time this round to individually root-cause each of those against `dev`, so I'm not asserting they're PR-caused, but they should be checked before this is considered CI-green even after Issue 1 is fixed. -- 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]
