DanielLeens commented on PR #11206:
URL: https://github.com/apache/seatunnel/pull/11206#issuecomment-5476981374
Thanks for the fast follow-up, @SEZ9. No new commit landed since my last
comment on this same head (`9577677bb86c`), so this is a reply rather than a
fresh review — but I did go re-check the two claims below directly against the
source rather than just cross-referencing issue numbers, since a couple of them
read differently than what's actually on this head.
**Conceding F3 outright — this is a real gap I missed.** I checked
`MySqlIncrementalSourceOptions.java:65-70`: `SCAN_NEWLY_ADDED_TABLE_ENABLED`
does default to `true`, confirmed in source, not just the docs. You're right
that this makes restore-time discovery opt-out rather than opt-in, which is
exactly the kind of default-value change our contribution guide calls out as a
hard constraint, and it's a real risk for any existing wildcard job restored
from checkpoint after upgrading onto this PR — an unplanned snapshot backfill
on tables the operator never asked to pick up. I'm adding this as a new
High-severity issue in my tracking and will flip the default to `false` in the
next commit. Thank you for catching this — my own passes never looked at the
option's default, only at the new-table registration path itself.
**Pushing back on F1/F8 with evidence — I think code-level enforcement
already exists.** I pulled `MySqlCatalogTableUtils.java` at the current head:
```
MySqlCatalogTableUtils.java:95-104 validateRuntimeTableIdentifiers(Table
table)
-> validateIdentifier("database"/"schema"/"table"/"column"/"primary key",
...)
MySqlCatalogTableUtils.java:109-116 validateIdentifier(type, identifier)
-> throws IllegalArgumentException if
!SAFE_IDENTIFIER.matcher(identifier).matches()
MySqlCatalogTableUtils.java:45 SAFE_IDENTIFIER =
Pattern.compile("[A-Za-z_][A-Za-z0-9_$]*")
```
This landed in `82165d5d18a1` (2026-08-27, before your last review on
`0bc52ddf500c`), so it wasn't on the head you last reviewed on 2026-08-23 — I
think that's why F1/F8 carried forward unchanged. Every runtime-discovered
database/schema/table/column/primary-key name is checked against this allowlist
*before* a `CreateTableEvent` is built, so an identifier containing a backtick,
semicolon, or whitespace is rejected in code, not just documented against. That
said, this doesn't make the concern moot: it's exactly what my own carried-over
Issue 1 is about — `validateIdentifier` throws uncaught, so the enforcement
exists but currently crashes the whole running job instead of skipping just the
offending table. I'd suggest we merge F1 into my Issue 1 rather than track it
separately, since the fix is the same (catch it in `handleTableChangeStruct`
and route the offending table through the multi-table failure policy instead of
letting the exception escape).
F8 stands as-is and is still accurate: `SAFE_IDENTIFIER` anchors on
`[A-Za-z_]`, so a legal MySQL table name like `2024_orders` would be rejected
and silently dropped with no backfill path. I'll widen the pattern to allow a
leading digit (while still rejecting all-digit names) in the same commit that
fixes Issue 1's crash behavior, and add the WARN-on-skip you asked for.
**F2 — agreed, and it's the same root cause as my own Issue 2**
(`MultiTableSinkWriter.java:506-508`, `hasSourceMatchedWriter` returning `true`
unconditionally whenever a runtime factory exists, no
`supportsNewlyCreatedTable()` gate ahead of it on Zeta). I'll fold your
suggested fail-fast `SeaTunnelException` naming the `TablePath` into that fix
rather than a silent drop.
**F4, F5, F6, F7 — no disagreement, all genuinely new and not yet in my
tracking.** F4 (routing/target-scope pinning for runtime-created tables) and F6
(checkpoint state format compatibility for `MultiTableState`, both the
pre-upgrade-restore-NPE direction and the rollback caveat) are the two I'd call
out as most important to land before this is mergeable, given they're both
about correctness under upgrade/restore, not just this feature's happy path. F5
(idempotent runtime writer creation) and F7 (JDBC sink docs) I agree are needed
too.
I'll fold F1 (merged into my Issue 1), F3, F4, F5, F6, F7, and F8 into the
next round of commits and post an updated issue list against the new head once
they land. Thanks for the thorough re-check — this round genuinely moved the PR
forward on the compatibility side, which is exactly where I hadn't been looking
closely enough myself.
--
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]
