SEZ9 commented on PR #11912: URL: https://github.com/apache/seatunnel/pull/11912#issuecomment-6460723121
Thanks for the detailed P1 write-up, @nzw921rx, on `d3a8a348ddb08c9866c305276ec8f91b01f0953f` — I'll leave that one to be handled in its own thread rather than fold it into this pass. For this pass I'm keeping to the points raised in the earlier review: **`MultiTableSink.java`** - **F1 / F2 / F7 (applier lifecycle):** `SchemaChangeApplier` instances are created eagerly, one per table per parallel writer, even when coordinated schema evolution is never triggered. This contradicts the SPI contract and multiplies the external connection footprint by parallelism × table count for the whole job lifetime. Could appliers be created lazily on the first schema-change event (or on the coordinator side only) and closed once the DDL is applied? If eager creation is required, please explain why here. - **F3 (target resolution / map keying):** `createSchemaChangeApplier(TablePath)` resolves the target sink nondeterministically when several logical tables map to the same physical sink table, and the applier map is keyed by logical fullName while the applier is built for the physical path. Please pick one key consistently (the physical path seems natural), make the resolution deterministic, and add a test for the many-logical-to-one-physical case. - **F8 (upgrade note):** please add a short note on checkpoint/savepoint restore behaviour for existing schema-evolution jobs after the Flink sink coordination changes. **`SchemaChangeApplier.java`** - **F4:** the SPI executes source-derived DDL but its contract says nothing about identifier safety. Please document in the javadoc that implementations must quote/validate identifiers (table, column, type names) rather than interpolating them raw into SQL. **`SupportCoordinatedSchemaEvolutionSink.java`** - **F5:** no documentation accompanies the new capability. Please add a concept doc and a section in the connector dev guide describing what a connector has to implement. - **F6:** the requirement that writers implement `SupportSchemaRefreshSinkWriter` is javadoc-only. Please enforce it (e.g. fail fast at sink/writer creation when the writer doesn't implement the interface) so a partially-implemented connector doesn't fail or silently skip the refresh mid-DDL. Happy to take another look once these have a reply or a push. <!-- streview-comment:2096 --> -- 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]
