DanielLeens commented on PR #11912: URL: https://github.com/apache/seatunnel/pull/11912#issuecomment-5370801666
Thanks for the second review pass, @SEZ9. I rechecked your findings against the current head (`199e786b2cf7`) — several of these are real and go beyond what I caught in my own round, so I'm merging them into the blocking list rather than leaving them as a separate open thread. **Issue 1/2/7 (eager per-table applier creation) — confirmed, High.** `MultiTableSink.createSchemaChangeAppliers()` (`MultiTableSink.java:232-266`) is called unconditionally from both `createWriter()` (`:156`) and `restoreWriter()` (`:225`), and its loop (`:235-251`) instantiates one `SchemaChangeApplier` per coordinated child sink on every call — i.e. once per parallel subtask, on every engine (Zeta and Spark also route through `createWriter`), regardless of whether schema evolution is ever used. This directly contradicts the SPI's own documented contract: `SupportCoordinatedSchemaEvolutionSink.java:46-48` states "The engine creates and invokes only the applier selected for the coordinated schema change." For a JDBC multi-table job that's parallelism × table-count live connection providers held for the job's lifetime. Agreed this needs to move to lazy, on-first-DDL creation. **Issue 3 (nondeterministic applier resolution + key mismatch) — confirmed, Medium.** `createSchemaChangeApplier(TablePath)` (`MultiTableSink.java:280-299`) iterates a `HashMap` and accepts the first entry matching *either* the logical key or the derived physical path (`:288-289`), while `createSchemaChangeAppliers()` keys its map by the logical `getFullName()` (`:248`). When multiple logical tables route to one physical sink table, which applier gets selected is iteration-order-dependent. Verified as described. **Issue 6 (writer-must-implement-SupportSchemaRefreshSinkWriter is unenforced until dispatch time) — confirmed, Medium.** There's no `instanceof` check anywhere in the translation layer (`SchemaOperator`, `SchemaEvolutionSinkFlow`, `ExternalSchemaChangeOperator`) at plan/construction time. The only enforcement is at `MultiTableSinkWriter.java:565-570`, which throws only when `dispatchSchemaRefreshToTargets` actually runs — i.e. mid-DDL, after post-change rows are already buffered behind the barrier, exactly as you described. Confirmed. I didn't independently re-verify Issues 4 (identifier-safety/SQL-injection contract gap), 5 (missing docs), and 8 (no incompatible-changes.md entry for restore behavior) line-by-line, but given the accuracy of 1/2/3/6/7 above I have no reason to doubt them, and they read as reasonable asks for a new public SPI. @dybyte — this reinforces "not ready to merge" beyond what I flagged: on top of my Issue 1 (the `MysqlCDCDriverResolver` compile failure blocking CI) and Issue 2 (dead `LocalSchemaCoordinator`), please also address SEZ9's Issue 1 (lazy applier creation — this one actively contradicts the SPI doc you wrote) and Issue 3 (deterministic, unambiguous applier resolution) as blockers. The checkpoint-barrier coordination design itself is still sound and I stand by that part of my review; these are implementation-level gaps in the MultiTableSink integration, not problems with the core approach. -- 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]
