SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5390960325
Thanks @DanielLeens for the thorough second pass and for independently verifying the fix on `9a7bac1e46` — good to see the `unit-test` jobs in run `32587393515` confirm that the destination-key collision (PR11077-F1) is resolved. Agreed that the opt-in `getPhysicalDestinationIdentifier()` with an instance-unique fallback is a safe default: connectors that don't opt in get no sharing. The remaining items from the previous round are still open: 1. **HIGH — restore-time state duplication (PR11077-F2):** snapshot still fans the shared writer's state out to every aliased identifier, and the union at restore hands the writer its state N times. Please snapshot under a single canonical identifier per destination key (or dedupe before restore) and add a restore test covering the aliased case. 2. **Schema/config divergence (PR11077-F3):** the shared writer is created from one arbitrary alias's sink instance/CatalogTable but receives rows from all aliases. Please add a fail-fast compatibility check or document why divergence is safe. 3. **proxyContexts registration (PR11077-F5):** the `containsValue` check only registers the first alias per destination, leaving other aliased identifiers without a context entry, and adds O(n²) startup cost. Please register a context for every alias and drop the linear scan. 4. **Docs (PR11077-F4, PR11077-F6):** the new `getPhysicalDestinationIdentifier()` SPI method, the writer-sharing behavior, and the changed `restoreWriter` contract (merged state from all aliased identifiers in one call) all need documentation for connector implementers. 5. **Error handling (PR11077-F7):** `IOException` from `createWriter`/`restoreWriter` is wrapped in an unchecked `RuntimeException` inside `computeIfAbsent`; please unwrap and rethrow so the declared failure contract holds. 6. **Minor (PR11077-F8):** add param/return tags to the `getDestinationKey` Javadoc for consistency with the package. Once F2 is fixed with test coverage and the docs land, the rest should be quick follow-ups. Thanks again for the careful verification work. <!-- streview-comment:516 --> -- 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]
