DanielLeens commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5674345945
Thanks for keeping this pinned down, @SEZ9 — but I want to flag a timing gap before this checklist gets re-run from scratch: my [09-14T09:38:39Z review](https://github.com/apache/seatunnel/pull/11077#pullrequestreview-5196154801) is a full from-scratch re-review of this exact head, `ef2bb0955f30bc108ef96d30e9c4adfe1f294279`, and it already re-verified F1-F8 line-by-line against the current source rather than carrying the pre-`a9d8464` checklist forward unchecked. Reposting the head-by-head mapping here since it looks like that review may have been missed: - F1 (destination-key collisions): resolved — `DestinationKey.equals()/hashCode()` (`MultiTableSink.java:621-670`) fold in `sink.getClass()` plus the connector-supplied identifier, falling back to raw object identity when either side has none. Regression test: `testSamePhysicalIdentifierDoesNotShareAcrossConnectorClasses`. - F2 (snapshot fan-out / restore-time union): resolved — snapshot persists under one canonical identifier, `getRestoredState()` (`MultiTableSink.java:392-404`) merges legacy per-alias checkpoints by content, not position. Covered by `testSharedWriterRoundTripRestoresOneCanonicalState` and `testRestoreMergesStateFromAllAliasedTables`. - F3 (schema/config divergence): resolved in `a9d8464` — `validateSharedDestinationSchemas()` (`MultiTableSink.java:129-161`) fails fast at construction on divergent `CatalogTable` schemas across aliases; the release-note callout is in the PR description. - F4/F6 (docs): resolved — `docs/en/developer/sink-connector-development.md` and the `zh` counterpart document `getPhysicalDestinationIdentifier()` and the merged-state `restoreWriter` contract. - F5 (`proxyContexts`/`containsValue`): not reproducible on this head — both `createWriter` and `restoreWriter` call `proxyContexts.put(...)` unconditionally per alias; there is no `containsValue` gate anywhere in that path. - F7 (`IOException` wrapped in `RuntimeException` inside `computeIfAbsent`): not reproducible — writer creation happens outside `computeIfAbsent`, and the checked exception propagates to the single outer `catch (IOException error)`. - F8 (Javadoc tags): resolved — `@param`/`@return` are present on `getDestinationKey`. None of this is a new claim — it's the same analysis you and I already walked through item-by-item on 09-10/09-11/09-12/09-13, re-verified fresh in the 09-14 full review rather than assumed carried-over. I also confirmed the current head (`ef2bb0955f3`) is a pure `dev`-sync merge with zero commits of this branch's own on top of `a9d8464` (diffed directly), so there's no new code for either of us to re-check right now — the F1-F8 set closed on `a9d8464` and stayed closed through the sync. If you're seeing something at a different `path:line` than what I've quoted, I'd genuinely like to see it — a pointer would help me recheck immediately. But as it stands, re-opening the full checklist without a concrete diff-backed pointer risks sending @hesam-oxe back over ground that's already been covered seven times. From my side this PR's own code has no open blockers; the only remaining gate is CI finishing green on the current run. -- 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]
