DanielLeens commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5775024021
@SEZ9 good timing - my from-scratch re-review of this same head (`1bfcea62b6`) landed a few hours before your comment and already works through F1 and F2 in the "Status of @SEZ9's F1-F8 at this head" section, so let me save you the duplicate pass and point at what is already answered versus what is genuinely still open. **F1 (destination-key collision detection)**: this is not left to logging alone. `validateSharedDestinationSchemas` runs in the `MultiTableSink` constructor, before both `createWriter` and `restoreWriter` (`MultiTableSink.java:112`, `:150-163`), and throws `IllegalStateException` at submit time when two aliases share a `getPhysicalDestinationIdentifier()` but their `TableSchema` differs - so a collision between aliases that are not the same physical destination is rejected, not just logged. That said, I do not think F1 is fully closed yet: my Issue 4 shows the file sink's own identifier only encodes `path + row-type` and omits bucket/endpoint/credentials/`tmp_path`/`file_name_expression`, so two aliases that genuinely point at different physical destinations but share path and row type can still collide silently today. That is the concrete gap behind your ask, tracked as a Medium blocker in my review, not resolved by the javadoc/log addition alone. **F2 (snapshot fan-out / restore-time duplication)**: state is recorded once per shared writer, not once per alias - `MultiTableSinkWriter.java:725-740` stores a single canonical record keyed by the first alias (`aliasedIdentifiers.get(0)`), and `MultiTableSinkWriterTest.java:664` (canonical round trip) plus `:829` (legacy per-alias state merge) both assert this directly: N aliases sharing one writer produce one state entry, and old per-alias checkpoints merge into that same single writer without re-adding duplicate state. The remaining edge I found (Issue 5) is narrower than double-counting: if the alias holding the canonical record is later removed or skipped before restore, the surviving aliases can restore with no state at all, which is a data-loss risk on a specific reconfiguration path rather than a fan-out duplication. **F3-F8 status line**: the same review section gives a one-line status for each, with source pointers - F3 is open (Issue 2: the check compares more than the write layout), F4/F6 are done (docs in both languages), F5 and F7 have no gap found, F8 is done (javadoc present). None of this changes the verdict - CI is still red and Issues 1-4 remain the real blockers - but hopefully it saves you re-deriving the F1/F2 analysis from the diff alone. Happy to re-verify anything above against the source if it does not match what you're seeing. -- 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]
