DanielLeens commented on PR #11077:
URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5645114571
@SEZ9 Verified your F2 question directly against the current head
(`153355ce8c`), plus a status recap since your last comment says F1/F4-F8 are
still unanswered in the thread (I think we're hitting the same client-side
rendering cutoff again — I did post per-item status for all of them on
09-10/09-11).
**F2 — confirmed: the current snapshot path persists under exactly one
canonical identifier, not fan-out.**
`MultiTableSinkWriter.snapshotState()` (lines 1342-1387):
```java
List<SinkIdentifier> aliasedIdentifiers = groupedEntry.getValue();
SinkIdentifier primaryIdentifier = aliasedIdentifiers.get(0);
...
snapshotStates.put(primaryIdentifier, states);
```
Only `primaryIdentifier` ever gets an entry in `snapshotStates` — the other
N-1 aliases sharing that writer get nothing written for them at snapshot time.
So on `getRestoredState()`'s all-aliases scan (`MultiTableSink.java:339-351`),
only one of the N aliases has a non-null entry; the rest are filtered out by
`.filter(Objects::nonNull)`. No duplication on a same-code round trip.
The `flatMap`-and-merge in `getRestoredState()` exists specifically for the
legacy case your question anticipated — the method's own Javadoc says it: "New
checkpoints store a shared writer state under one canonical identifier. Earlier
checkpoints can contain a distinct state list for each alias, which still must
be restored together." That's restoring state from N *formerly independent*
writer instances (pre-this-PR, before sharing existed) into the *now-shared*
writer via `restoreWriter(context, List<state>)` — which is the same
multi-state-merge contract SeaTunnel already relies on for parallelism-rescale
restores, not a new fragile mechanism.
Both directions are already covered by dedicated tests on this head, so the
test you asked for already exists:
- `testSharedWriterRoundTripRestoresOneCanonicalState`
(`MultiTableSinkWriterTest.java:662-700`) is exactly what you described: two
aliases (`src.db.t1`, `src.db.t2`) share one writer, `snapshotState()` is
asserted to produce exactly 1 state entry (`states.get(0).getStates().size()`
== 1), then the round trip through `restoreWriter` is asserted to hand the
connector exactly 1 restored state entry (`restoredStates.size()` == 1).
- `testRestoreMergesStateFromAllAliasedTables` (`:735-771`) covers the
legacy/fan-out case explicitly: constructs a `MultiTableState` with two
*separate* per-alias entries (simulating an old checkpoint), and asserts
`restoreWriter` merges both into one list of 2 (`mergedStates.size()` == 2,
containing both), confirming the backward-compat path doesn't drop or silently
duplicate anything.
On the reordering follow-up: I re-checked `identifiersByDestinationKey`
(`MultiTableSink.java:225-237`) — it's a content-keyed map built fresh at
restore time from `sinks.keySet()`, and `getRestoredState` does a full scan
over it rather than indexing by position, so which alias `groupByIdentity`
happens to pick as "primary" is irrelevant to whether the state is found. No
reordering-specific test exists, but given the lookup is structurally
order-independent (not just "usually works"), I don't think one is load-bearing
here — happy to be shown otherwise.
**Status recap for F1, F4-F8** (reposting since your 09-12 comment says
these are still open in the thread — full detail is in my 2026-09-11T10:26:38Z
comment, id 5633072441, in case that one also got cut off on your side):
- F1 (destination-key collision): Resolved —
`DestinationKey.equals()`/`hashCode()` fold in `sink.getClass()` +
`physicalDestinationIdentifier`, falling back to object identity when either
side has none. Regression test:
`testSamePhysicalIdentifierDoesNotShareAcrossConnectorClasses`.
- F4 (undocumented `getPhysicalDestinationIdentifier()` SPI): Resolved —
documented in `docs/en/developer/sink-connector-development.md` and the `zh`
counterpart.
- F5 (`proxyContexts` only registers first alias via `containsValue`): Not
reproducible on `153355ce8c` — both `createWriter` and `restoreWriter` call
`proxyContexts.put(...)` unconditionally per alias, no `containsValue` gate in
this path.
- F6 (undocumented `restoreWriter` merged-state contract): Resolved, same
doc update as F4.
- F7 (`IOException` wrapped as unchecked inside `computeIfAbsent`): Not
reproducible — the connector's `createWriter`/`restoreWriter` call sits in a
plain `if (writer == null) {...}` block inside the outer `try`, not inside a
`computeIfAbsent` lambda; the checked exception propagates to the single outer
`catch (IOException error)`.
- F8 (missing Javadoc tags on `getDestinationKey`): Resolved —
`@param`/`@return` present.
If F5 or F7 are reproducible against a different line range than what I'm
quoting, a `path:line` pointer would help — we should both be on `153355ce8c`.
**Separately, re-flagging the commit-authorship issue since it's still
present and shouldn't get dropped just because it's come up before:** two
commits on this exact head are messaged `[Chore] Retrigger CI` and resolve via
the GitHub API to `author_login: DanielLeens` (`343205baac64`, `26dd37e11016`),
both carrying committer/author email `[email protected]`. I checked this
myself just now rather than taking the prior finding at face value: that email
*is* the real DanielLeens account's registered email (`gh api
users/DanielLeens` returns it directly), which is exactly why GitHub's UI/API
attributes those commits to me — but `gh api repos/hesam-oxe/seatunnel --jq
.permissions` shows `"push": false` for my own token against this fork, so my
account genuinely could not have pushed them. Setting a commit's
author/committer email is pure local `git config`, not an authentication check,
so this doesn't require my credentials — whoever has actual push access to
`hesam-oxe/seatu
nnel` (the fork owner or a collaborator) can set that metadata to anything and
push it under their own access. That means two commits on this PR are
fabricated to look like they came from a maintainer reviewer account that never
touched this repository. This is still unresolved at `153355ce8c` and, per the
prior rounds' notes, is now a recurring pattern on this specific PR — worth a
maintainer's attention independent of the code review outcome, and I don't
think it should be merged past without that being explicitly acknowledged.
--
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]
