SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-6514027927
Quick follow-up on where this PR stands from my side. The current head `8b383c04eeed65c41a14ba756100a33f430124eb` is unchanged since the previous review, so I can't confirm anything as resolved yet. The remaining points are: - **F1 (HIGH, Security)** – `MultiTableSink`: a destination-key collision can silently route one table's rows through another sink's writer. Please add a guard (or an explicit, documented contract) so two logically different destinations never share a writer unintentionally. - **F2 (HIGH, Bug)** – `MultiTableSink`: snapshotting the shared writer's state under every aliased identifier and unioning on restore duplicates that state N times. Please emit the shared state once (or de-duplicate on restore) and cover it with a snapshot/restore test. - **F3 (MEDIUM)** – `SeaTunnelSink`: the shared writer is built from an arbitrary alias's sink/`CatalogTable` but receives rows from all aliases. Please validate schema/config compatibility across aliased tables, or fail fast when they diverge. - **F4 / F6 (MEDIUM, Docs)** – `getPhysicalDestinationIdentifier()`, the writer-sharing behavior, and the changed `restoreWriter` contract (merged state from all aliased identifiers in a single call) need documentation for connector implementers under `docs/`. - **F5 (MEDIUM)** – `proxyContexts` only registers the first alias per destination via `containsValue`, leaving aliased identifiers without a context entry and making startup O(n²). Please register a context for every identifier and drop the `containsValue` scan. - **F7 (MEDIUM)** – `IOException` from `createWriter`/`restoreWriter` is wrapped in an unchecked `RuntimeException` inside `computeIfAbsent`. Please restructure so the declared checked exception propagates. - **F8 (LOW)** – add param/return tags to the `getDestinationKey` Javadoc. Two housekeeping items: GitHub reports the branch as `diverged` from `dev` with mergeable state `dirty`, so please sync and resolve the conflicts while preserving both sides' intended changes. Separately, the `Build` check on this head is `ACTION_REQUIRED` — please start the existing workflow on your fork rather than pushing an empty commit to trigger it. Once the fixes are pushed, the branch is synced, and the Build run is linked, drop the new head SHA here and I'll do a complete re-review. Thanks for the work on this! <!-- streview-comment:2139 --> -- 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]
