SEZ9 commented on PR #11912: URL: https://github.com/apache/seatunnel/pull/11912#issuecomment-6594364779
Following up on the earlier review findings against head `d3a8a348ddb0`, using the status mapping above as the reference: - **F1 / F2 / F7 (eager per-writer, per-table applier creation):** The description of lazy creation — `createWriter()` / `restoreWriter()` no longer building appliers, and `createSchemaChangeApplier(TablePath)` only running when a coordinated DDL is dispatched — resolves the lifecycle and footprint concerns as stated. I'm happy to close these once the rebased head is pushed and I can confirm nothing in `MultiTableSink` regressed during conflict resolution. - **F3 (nondeterministic resolution / inconsistent keying):** Logical-key lookup followed by `resolvePhysicalSinkTable(...)` that throws `IOException` on ambiguous physical matches, with the applier created for the resolved physical path, is the behavior I was after. Please make sure the ambiguity check survives the merge with `dev`. - **F4 (identifier safety in the `SchemaChangeApplier` SPI):** Covering this in the `SchemaChangeApplier.apply()` javadoc is sufficient for the SPI contract. - **F6 (unenforced `SupportSchemaRefreshSinkWriter` contract):** Fail-fast in `FlinkSink` addresses the Flink path. One remaining question: is the same check applied on the other engine sink wrappers, or is Flink the only place a coordinated-support sink can reach a writer without `SupportSchemaRefreshSinkWriter`? If it is Flink-only by design, a short note in the `SupportCoordinatedSchemaEvolutionSink` javadoc would be enough. - **F5 / F8 (docs and upgrade note):** en/zh updates to `sink-connector-development.md`, `schema-evolution.md` and the checkpoint/savepoint restore note in `incompatible-changes.md` cover both findings. Concrete asks before I mark these resolved: 1. Rebase onto latest `dev` and push the new head so the above can be re-verified on actual code rather than the summary. 2. After the rebase, confirm the `MultiTableSink`, `SchemaChangeApplier` and `SupportCoordinatedSchemaEvolutionSink` changes are neither dropped nor duplicated. 3. Answer the F6 question about enforcement outside `FlinkSink`. Nothing else from my previous review remains open. <!-- streview-comment:2248 --> -- 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]
