SEZ9 commented on PR #12807: URL: https://github.com/apache/seatunnel/pull/12807#issuecomment-6460625463
Thanks @nzw921rx for the thorough local verification on `c3437e23a79733fe996fdcb14d647ce596488178` with `dev` at `403efeee0b3806c77e55c38368abdd41fbf828bd`. Your call-chain summary matches my reading: normal commits still go through `commit()`, restores now go through each table's `restoreCommit()`, and committers without an override fall back to the interface default. A few items before this merges, mapped to the earlier review points: **Tests (F5 / F6)** – The eight supplemental wrapper tests you describe (per-table routing, unequal non-empty retry lists with table identifiers preserved, default `restoreCommit()` → `commit()` delegation, empty restored state) are exactly the coverage I asked for. Since they were only run locally, please add them to `MultiTableSinkAggregatedCommitterTest` in this PR so the restore path stays protected going forward. **Fail-fast on restore (F1)** – "Exceptions remaining visible, with the existing fail-fast behavior" confirms the current behaviour but doesn't resolve the concern: when one table throws, the remaining tables are never restored and the propagated exception carries no table identifier. I'm fine with either (a) wrapping the exception with the table identifier in this PR, or (b) explicitly deferring it to a follow-up. Please say which. **Iceberg (F4)** – 131 Iceberg unit tests passing is good, but the real recovery runs described are MySQL-CDC → JDBC only. This PR makes `IcebergAggregatedCommitter.restoreCommit` reachable under Zeta for the first time, so please either run the existing Iceberg savepoint IT against this head and report the result, or note it as explicitly unvalidated. **Docs (F2 / F3)** – Not mentioned yet: the stale `restoreCommit` signature in the architecture doc and a release/upgrade note for the JDBC XA / Iceberg restore behaviour change (including correcting the existing JDBC XA entry in `incompatible-changes.md`) are still open. **Style (F7 / F8)** – Minor: move the `//` explanation above the override annotation into Javadoc and use `List<?>` instead of raw `List` on the changed line. **CI** – Run `40507160077` / job `122200579062` was cancelled on timeout rather than failing, so I'd prefer a re-trigger to get a completed run on record rather than bypassing, given the API module is in the blast radius. Also, the negative-controls table in your comment appears cut off after the header row — could you repost the dev-control vs. PR results? <!-- streview-comment:2095 --> -- 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]
