SEZ9 commented on PR #11206: URL: https://github.com/apache/seatunnel/pull/11206#issuecomment-6183874493
Thanks for the detailed per-finding status at `9007896511517e4b874142cfe704102861bbc444`. Notes from my side; I'll verify each point against the diff once the next push is up. **F1 / F8 (identifier validation)** — Your description of `MySqlCatalogTableUtils.toRuntimeCatalogTable` → `validateRuntimeTableIdentifiers` as the single validation boundary, using the `[A-Za-z0-9_$]*[A-Za-z_$][A-Za-z0-9_$]*` pattern, sounds like the right direction. Two asks: - Since the guarantee rests on one boundary, please add a unit test asserting that a `CreateTableEvent` whose identifier contains e.g. a backtick or `;` is skipped and never reaches the sink, so the invariant can't silently regress if another converter is wired in later. - Please make sure the MySQL-CDC source doc describes the pattern actually enforced in code, including that all-numeric names are rejected and skipped with a WARN. **F2 (fail-fast contract)** — Thanks for confirming the Javadoc update is coming. Please state explicitly on both `SupportMultiTableSinkWriter#supportsNewlyCreatedTable` and `TableSinkFactory#supportsNewlyCreatedTable` that a runtime `CreateTableEvent` is never silently dropped and that the job fails when the sink has not opted in. **F3 (`scan.newly-added-table.enabled` default)** — Not covered in your reply. Could you say whether you intend to keep the default as `true`? If so, please add an upgrade note to the MySQL-CDC docs describing the changed checkpoint-restore behavior for existing wildcard jobs; otherwise flip the default to `false`. **F7 (JDBC sink docs)** — Thanks; making `exactly_once = false` explicit alongside `generate_sink_sql = true` in both the en and zh JDBC sink docs sounds good. **F4 / F5 / F6** — Understood these are still open. For F6, please include a test that restores an old-format `MultiTableState` (without runtime-registered tables) so rollback/restore behavior is covered, not just documented. Since compilation and tests weren't run for this update, please ping once the next push is up and the suite has been exercised locally, and I'll do another pass. <!-- streview-comment:1773 --> -- 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]
