SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5724729100
Thanks for the source-level walkthrough on `ef2bb095` — reactions item by item, plus what I'd still like to see before resolving. **F1 (destination-key collisions)** — Agreed that the `getPhysicalDestinationIdentifier()` javadoc (from `a9d846469b`) puts the burden on the connector to fold every distinguishing coordinate into the identifier. My remaining concern is that `DestinationKey.equals()` only compares the sink class and the identifier string, so a connector that gets this wrong silently routes one table's rows through another destination's writer. Two asks: (1) have the javadoc explicitly call out credential divergence as a collision hazard (the quoted wording lists endpoint/warehouse/namespace/table/branch but not credentials), and (2) add a log line where a second alias joins an existing shared writer so operators can see when sharing kicks in. If (2) feels out of place in `MultiTableSink`, say so and I'll settle for (1). **F2 (state fan-out + restore union)** — Good correction on the premise; I accept that `getRestoredState(...)` is a plain `flatMap` union with no dedup, and that `testRestoreMergesStateFromAllAliasedTables` pins the 2-alias/2-distinct-state case. The finding wasn't about dedup, though — it's that snapshot fans the *same* shared-writer state out to every aliased identifier, and restore then unions them back, so the connector receives N copies of one writer's state. Either (a) snapshot the shared writer's state under a single canonical identifier per destination, or (b) keep the fan-out but document in the `restoreWriter` javadoc that identical entries may appear N times and the connector must tolerate that, with a coordinator test using a third alias carrying an identical state that asserts the resulting list length. Which direction do you prefer? **F3 (schema divergence across aliases)** — The comment cuts off mid-sentence at `validateSharedDestinationSchemas()`, so I can't see what it checks. Could you finish the description or point me to the test covering a schema mismatch between two aliases sharing a destination? Specifically: does it compare full column definitions or only names/count, and is it also invoked on the restore path? **F8** — Not reached in the comment; if the param/return tags on `getDestinationKey` are already in `ef2bb095`, just confirm and I'll close it. **F4 / F5 / F6 / F7** — Not addressed in this round, so still open from my side: user-facing docs for the new SPI method and writer-sharing behavior (F4), the `containsValue`-based `proxyContexts` registration that skips aliased identifiers (F5), documenting the merged-state `restoreWriter` contract for implementers (F6), and the `IOException` → `RuntimeException` wrapping inside `computeIfAbsent` (F7). Happy to look at those in the per-item walkthrough when it lands. <!-- streview-comment:1143 --> -- 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]
