DanielLeens commented on PR #11912:
URL: https://github.com/apache/seatunnel/pull/11912#issuecomment-6391361255
Thanks @SEZ9 for the follow-up. I re-checked each "remaining ask" against
the current head (`d3a8a348ddb0`) and most of them are already in the branch
(no new commit since my last review, so this is the same code you can look at
now):
- **Lazy applier creation** - done. `MultiTableSink.createWriter()` /
`restoreWriter()` no longer build appliers.
`createSchemaChangeApplier(TablePath)` (`MultiTableSink.java:253-289`) builds
exactly one applier for the selected table, only when the engine dispatches a
coordinated DDL. Jobs that never hit a schema change create none, so there is
no parallelism x table-count footprint.
- **Deterministic resolution and consistent keying** - done. The method
first looks up the logical key, then falls back to a physical-path match via
`resolvePhysicalSinkTable(...)`. If more than one logical table maps to the
same physical table it throws an `IOException` listing the sorted logical
tables instead of picking the first `HashMap` hit. The applier is always
created for the resolved physical path.
- **Identifier-safety javadoc** - done in `SchemaChangeApplier.apply()`:
event-derived identifiers are untrusted, must be validated/quoted through the
sink dialect, and must not be concatenated raw into DDL. The same javadoc also
states that re-delivery after recovery must be idempotent.
- **Writer contract enforcement** - done. `FlinkSink.java:103-120` fails
fast when a sink that declares coordinated support creates a writer that does
not implement `SupportSchemaRefreshSinkWriter`, closing the writer first.
- **Docs and upgrade note** - done in both languages:
`docs/{en,zh}/developer/sink-connector-development.md`,
`docs/{en,zh}/introduction/configuration/schema-evolution.md`, and the
checkpoint/savepoint restore note in
`docs/{en,zh}/introduction/concepts/incompatible-changes.md`.
The one item that is genuinely still open is the small one we agreed on in
the inline thread: a javadoc line on
`SupportCoordinatedSchemaEvolutionSink.supportsCoordinatedSchemaEvolution()`
explaining why a plain `instanceof` is not enough (`MultiTableSink` implements
the interface itself, so the composite must answer "all children support it").
It is non-blocking. @dybyte, could you add that line when you next push?
Separately, the apache-side `Build` check is red only because there is no
`Build` run in the fork for this head (the previous head had a fully green fork
run). @dybyte, please enable GitHub Actions workflows in your fork and
push/re-trigger so the check can run on `d3a8a348ddb0`.
If you see a specific scenario in the code above that I mis-traced, please
point me at the line and I will re-check. Thanks again for the careful review!
--
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]
