SEZ9 commented on PR #11912: URL: https://github.com/apache/seatunnel/pull/11912#issuecomment-5385644471
Thanks for the thorough recheck against `199e786b2cf7`, @DanielLeens — merging these into the blocking list rather than tracking them separately works for me, and your verification notes match what I found. On the confirmed items, here's what I'd consider done vs. still open: 1. **Issues 1/2/7 (eager per-table applier creation)** — glad we agree on the severity. The concrete ask is to move applier creation to lazy, on-first-DDL instantiation so it happens only when a coordinated schema change is actually dispatched, consistent with the contract stated in `SupportCoordinatedSchemaEvolutionSink.java:46-48`. That resolves both the SPI contradiction and the parallelism × table-count connection footprint. 2. **Issue 3 (nondeterministic applier resolution)** — your analysis of the `HashMap` iteration plus the logical-vs-physical key mismatch is exactly the problem. Please make resolution deterministic: pick one keying scheme (I'd suggest keying consistently by physical path if that's what the applier operates on) and either resolve unambiguously or fail fast when multiple logical tables map to one physical sink table. 3. **Issue 6 (unenforced writer contract)** — since the only enforcement today is the throw at `MultiTableSinkWriter.java:565-570` mid-DDL, please add a construction-time check so a connector implementing `SupportCoordinatedSchemaEvolutionSink` without `SupportSchemaRefreshSinkWriter` fails at job planning rather than after post-change rows are buffered. 4. **Issues 4/5/8** — no need for line-by-line re-verification; these are contract/documentation asks rather than code-behavior claims. Concretely: (4) add identifier-safety requirements to the `SchemaChangeApplier` javadoc so implementors know source-derived identifiers must be quoted/validated, (5) add docs covering the new coordinated schema evolution capability (concept + connector dev guide), and (8) add an upgrade note covering checkpoint/savepoint restore behavior for schema-evolution jobs. Nothing beyond these — once you push the fixes, ping me with the new head and I'll do a focused re-review against just these items. <!-- streview-comment:478 --> -- 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]
