DanielLeens commented on PR #11077: URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5881991249
Thanks for the thorough status update, @hesam-oxe, and for confirming the CI diagnosis matches what I'd already found. One clarification so we don't talk past each other: my last full re-review (2026-09-20, on this exact head `1bfcea62b6`) already confirmed F1 and F2 resolved at the coordinator level - `DestinationKey` folding in the connector class plus `getPhysicalDestinationIdentifier()`, and one canonical state record per shared writer via `groupByIdentity` - so nothing in your summary is new information there, it's good to have it written down in one place though. F4-F6 (docs, `proxyContexts`, `restoreWriter` contract) and the `CONTINUE_OTHER_TABLES` quarantine fix were likewise already accounted for. The four items that are actually still open are my Issues 1-4 from that same review, and none of them is covered by anything in this summary (nor by any new commit - the head is still `1bfcea62b6`, unchanged since I last reviewed it): - **Issue 1**: the startup fail-fast and file-layout change for existing jobs aren't recorded in `docs/en|zh/introduction/concepts/incompatible-changes.md`. - **Issue 2**: `validateSharedDestinationSchemas` compares the full `TableSchema` (including things like column length, scale, comment, constraints) rather than the row layout the file sink actually writes, so it both under- and over-fires relative to what the PR description promises. - **Issue 3**: `MultiTableSink.java:229`/`:347` log `getPhysicalDestinationIdentifier()` verbatim at INFO, while the contract now steers implementers toward folding credentials into that identifier - that's a secret-in-logs path for the next connector to opt in. - **Issue 4**: `BaseMultipleTableFileSink.getPhysicalDestinationIdentifier()` only encodes `path + row-type`, so two aliases that differ in bucket, endpoint, `tmp_path` or credentials but share path and row type can still be merged into one writer with no error - this is the concrete gap behind F1, not closed by the coordinator-level fix alone. None of these need a redesign, but they do need a commit before this is mergeable. On CI: worth flagging that the fork run you linked (35497458667) has since finished, and the conclusion is still `failure`, not `in_progress` - 5 jobs are red after the reruns: `engine-v2-it (11)`, `all-connectors-it-2 (8)` and `(11)`, `all-connectors-it-7 (8)` and `(11)`. That said, those are the same categories I already diagnosed as unrelated in my last review (Opengauss CDC restore-timeout, the minio image-pull failure, and an engine checkpoint-failover test), and two other jobs that were red then (`engine-v2-it (8)`, `all-connectors-it-1 (11)`) are green now - so the rerun did help, just not all the way. The apache-side `Build` check is still failing as of right now. Happy to re-review promptly once a commit addressing Issues 1-4 lands. -- 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]
