DanielLeens commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5713081588
@SEZ9 jumping in on F1/F2/F3/F8 with what I can verify directly from the current head (`ef2bb095`) while @hesam-oxe puts together the per-item walkthrough — no new commit since either of our last comments, so this is source-reading, not a re-review. **F1 (same connector class, different config, same identifier string)** — this is already answered in the contract itself, not left implicit: `SeaTunnelSink.getPhysicalDestinationIdentifier()`'s javadoc states "An implementation must include every connection coordinate that distinguishes a physical destination, such as endpoint, warehouse, namespace, table, and branch where applicable" (`seatunnel-api/src/main/java/org/apache/seatunnel/api/sink/SeaTunnelSink.java`, the paragraph right above the method, added as part of the `a9d846469b` commit). So yes — sharing is driven purely by the returned identifier string plus connector class (`DestinationKey.equals()` in `MultiTableSink.java` only compares `sink.getClass()` and the identifier, never sink config), and the contract explicitly makes it the connector implementor's job to fold in whatever config differs (credentials/endpoint) so two differently-configured instances don't collide. Nothing left to add in code here; it's a docu mented connector responsibility, same as you suggested. **F2 (mixed legacy dedup)** — worth correcting the premise before hunting for a test name: `MultiTableSink#getRestoredState(...)` does not do content-based/equals dedup at all. It's a plain `flatMap` union of every aliased `SinkIdentifier`'s state list (see the method body, ~15 lines, no `equals`/`distinct` call anywhere). `testRestoreMergesStateFromAllAliasedTables` (`MultiTableSinkWriterTest.java`) already shows this: two aliases with two genuinely distinct states merge into a 2-entry list, nothing collapsed. Since there's no collapsing logic, a hypothetical third alias carrying an identical duplicate would simply add a third entry, not vanish — matching `restoreWriter`'s javadoc, which puts the "tolerate a state list whose entries originate from different source tables" burden on the connector, not the coordinator. So a dedicated "mixed" coordinator-level test wouldn't be pinning any coordinator behavior that doesn't already exist; if you want duplicate-tolerance pinned, th at belongs in a connector-specific restoreWriter test, not here. **F3** — `validateSharedDestinationSchemas()`, called from the constructor, throws `IllegalStateException` naming both table paths and the shared destination on schema mismatch. Covered by `testSharedDestinationWithDivergentSchemasFailsFast` and `testSharedDestinationWithCompatibleSchemasSharesOneWriter`. **F8** — `getDestinationKey`'s javadoc already has both `@param` tags and `@return`; nothing missing on this head. -- 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]
