SEZ9 commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5629230297
Thanks for going back through head `153355ce8c` item by item — here's where I land on each. **F1 (destination-key collision, HIGH)** — `DestinationKey.equals()`/`hashCode()` folding in `sink.getClass()` plus the connector-supplied `physicalDestinationIdentifier`, with a fall-back to raw object identity when either side has none, is what I was asking for, and `testSamePhysicalIdentifierDoesNotShareAcrossConnectorClasses` in `MultiTableSinkWriterTest.java` is the evidence I wanted. I'll run the test locally, but I consider this addressed. **F2 (restore-time state duplication, HIGH)** — Driving each distinct writer once via `groupByIdentity` and recording state under a single canonical identifier (`aliasedIdentifiers.get(0)`) resolves the fan-out/union concern. One follow-up: the canonical identifier depends on the position in `aliasedIdentifiers`, so could you confirm that ordering is stable between a snapshot on one run and a restore on another (e.g. after a config reorder)? If the round-trip test in `MultiTableSinkWriterTest.java` already covers a reordered alias list, just point me at it. **F3 (schema/config divergence, MEDIUM)** — Folding the row type into `BaseMultipleTableFileSink#getPhysicalDestinationIdentifier()` is a reasonable guard for the file sink, but it's a per-connector mitigation rather than a guarantee in `MultiTableSink`. Since the SPI is public, a future connector that opts in gets no protection. I'd like either (a) a defensive check in `MultiTableSink` that the `CatalogTable` schemas of all aliases mapping to the same `DestinationKey` are compatible, failing fast with a clear error, or (b) an explicit note in the `getPhysicalDestinationIdentifier()` Javadoc that implementers must encode every schema/config dimension that affects write compatibility. (a) is my preference. **F4–F8** — The comment as I see it stops partway through F3, so I don't have the status for the remaining items. Could you post the rest (or a short per-item status) covering: - F4: docs under `docs/` for the new `getPhysicalDestinationIdentifier()` SPI method and the writer-sharing behavior. - F5: `proxyContexts` only registering the first alias per destination via `containsValue`, and the O(n²) startup cost. - F6: documenting the changed `restoreWriter` contract (merged state from all aliased identifiers passed to one call) for connector implementers. - F7: the checked `IOException` from `createWriter`/`restoreWriter` being wrapped in `RuntimeException` inside `computeIfAbsent`. - F8: param/return tags on the `getDestinationKey` Javadoc. Once those are covered, with the same file/test pointers you gave for F1/F2, I'll do a final pass on `153355ce8c`. <!-- streview-comment:962 --> -- 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]
