SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5612010991
Thanks @DanielLeens for the transparent scope note. I agree with your read: `153355ce8c` is the same head you approved on 2026-09-05, so there is no new diff to evaluate, and what changed is the Build signal. Your conclusion that the failure is real but not caused by this PR is useful, but your comment appears cut off on my side after "substantiv", so I can't see the findings you refer to. Could you re-post (or link) the part that shows why the Build failure is unrelated to this PR, so I can cross-check it before treating the red signal as pre-existing? Independently of the CI question, the previously raised review points are still open from where I sit, and an unchanged head doesn't resolve them: - **F1 (HIGH, security):** a destination-key collision between sinks that are not actually the same destination can silently route one table's rows through another sink's writer (`MultiTableSink.java`). - **F2 (HIGH, bug):** snapshot fan-out of the shared writer's state to every aliased identifier, combined with the restore-time union, duplicates that state N times on recovery. - **F3 (MEDIUM):** the shared writer is created from an arbitrary alias's sink/`CatalogTable` but receives rows from all aliases; schema/config divergence across shards is unguarded (`SeaTunnelSink.java`). - **F5 (MEDIUM):** `proxyContexts` only registers the first alias per destination via `containsValue`, leaving aliased identifiers without a context entry and adding O(n²) startup cost. - **F7 (MEDIUM):** the `IOException` from `createWriter`/`restoreWriter` is wrapped in an unchecked `RuntimeException` inside `computeIfAbsent`, breaking the declared failure contract. - **F4 / F6 (MEDIUM, docs):** `getPhysicalDestinationIdentifier()` and the changed `restoreWriter` contract (merged state from all aliased identifiers in one call) are not documented under `docs/` or for connector implementers. - **F8 (LOW):** `getDestinationKey` Javadoc is missing param/return tags. If any of these were already resolved somewhere I don't have visibility into, please point me to it. Otherwise I'd like to see F1 and F2 in particular addressed in a new commit, since they affect correctness for exactly the 200+ shard use case this PR targets. <!-- streview-comment:943 --> -- 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]
