SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-6183499737
Thanks @hesam-oxe for the checklist against `8b383c0` and for running the multi-table sink suite locally — the per-finding mapping makes this easy to follow. I'll re-review the diff for each item; a few things I'd like to confirm along the way: - **PR11077-F1 (key collisions)** — you mention a class + identifier key with an identity fallback. When does the identity fallback apply, and are two sinks with *different* configs but the same `getPhysicalDestinationIdentifier()` still folded together? If so, that's the credential-folding case the warning is meant to flag; a unit test asserting the warning path (or asserting they are *not* folded) would make this concrete. - **PR11077-F2 (snapshot/restore duplication)** — `groupByIdentity` plus snapshot-once-per-writer under `primaryIdentifier` sounds right. The case I'd still like covered by a test is legacy restore: a checkpoint taken before this change (per-alias state for N aliases) restored onto the shared writer should yield exactly one copy of the state, not N. If that isn't in `MultiTableSinkWriterTest` today, please add it. - **PR11077-F3 (schema divergence)** — fail-fast on `SeaTunnelRowType` mismatch is good. Please confirm the error message names both table identifiers involved so users can tell which shard diverged. - **PR11077-F4 / F6 (docs)** — thanks for the "Multi-Table Writer Sharing" section. Please make sure it explicitly states the new `restoreWriter` contract (one call receiving merged state from all aliased identifiers), since that's the part connector implementers are most likely to get wrong. - **PR11077-F5 / F7 / F8** — the described changes (unconditional `put` per alias, explicit get/put around `createWriter`/`restoreWriter` so `IOException` propagates, `param`/`return` Javadoc tags on `getDestinationKey`) match what was raised. I'll confirm each against the diff before marking them resolved. On your offer to add a multi-table test case: yes, the alias-join + legacy-restore scenario above is the one addition I'd ask for before this moves forward. <!-- streview-comment:1767 --> -- 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]
