DanielLeens commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5476957744
Thanks both — @davidzollo for the recheck and @SEZ9 for continuing to track this against every new head. I independently re-pulled `MultiTableSink.java` and `MultiTableSinkWriter.java` at the current head `c044430043bb` (not from the diff, the raw file content) to settle the disagreement on F1/F2 before replying, since the two comments point in opposite directions. **F1 (destination-key collision):** confirmed fixed on this head. `getDestinationKey()` (`MultiTableSink.java:384-387`) now returns a `DestinationKey` object, not a raw string. Its `equals`/`hashCode` (`MultiTableSink.java:587-615`) namespace by `sink.getClass()` + `physicalDestinationIdentifier` + `replicaIndex`, and fall back to sink-instance identity (`sink == that.sink`) whenever either side has no identifier. That's exactly @SEZ9's suggested fix (1) from the 2026-08-23 round. @davidzollo's read is accurate. **F2 (restore-time state duplication):** also confirmed fixed on this head. `MultiTableSinkWriter.snapshotState()` (`MultiTableSinkWriter.java:716-761`) now groups aliased identifiers by writer identity via `groupByIdentity` and does `snapshotStates.put(primaryIdentifier, states)` — only the first alias in each group gets an entry, so a checkpoint taken with this code has exactly one state list per shared writer. `MultiTableSink.getRestoredState()` (`MultiTableSink.java:338-349`) still unions across every aliased identifier, but since only the primary identifier resolves to a non-null entry in a checkpoint written by this code, the union collapses to one copy — the fan-out/duplication only happens for checkpoints written by an *older* revision, and that union is kept specifically for that backward-compat case (see the comment directly above the loop in `snapshotState`). This is also exactly @SEZ9's suggested fix. @SEZ9 — the specific evidence lines quoted for F1/F2 in the 2026-08-31 pass (`return destinationIdentifier + "_" + replicaIndex;`, and the old `MultiTableSink.java:259/274/279` flatMap snippet) don't exist anywhere in the current head; I grepped for that exact string and it's not present. Given F4-F8 in the same comment were clearly re-verified fresh against `c044430043bb`, I suspect the F1/F2 sections got carried forward unchanged from the 2026-08-23 review rather than re-diffed against this head. Could you take one more look and confirm whether you're intentionally re-opening these against the current implementation, or whether they can be marked resolved like F4-F8? Happy to walk through the current code together if I'm missing something. F3 (schema/config divergence across aliased sinks) is a fair carryover either way — the javadoc contract was added (`SeaTunnelSink.java:150-166`, "sinks that return the same identifier must have equivalent write schema, write settings, and commit semantics"), but there's still no fail-fast check comparing `getWriteCatalogTable()` across aliases sharing a destination key, so I'm keeping that one open as Medium/non-blocking, consistent with my 2026-08-29 pass. No new commit landed since my last note, so this stays a reply rather than a fresh review. The open item blocking merge from my side is still just the reported conflict against `dev`. -- 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]
