SEZ9 commented on PR #12807: URL: https://github.com/apache/seatunnel/pull/12807#issuecomment-6432914551
Re-checked `d9e1a8a06722` against the previous round on `3c333b52ee`: the two PR files are unchanged (the new head is another `dev` merge), so the earlier points still stand. The core change looks right — the wrapper now forwards `restoreCommit` per table instead of letting the interface default downgrade it to `commit` — and I found no new blocker. Remaining asks, in priority order: **Should be addressed before merge** 1. **Fail-fast in `restoreCommit` (MEDIUM, Robustness).** In `commitPerTable(infos, true)`, the first table whose `restoreCommit` throws aborts the loop, so the remaining tables' in-doubt XA transactions stay prepared, and the propagated exception carries no table identifier. Please either continue over the remaining tables and rethrow an aggregated exception afterwards, or at minimum wrap the exception with the `sinkIdentifier` so the failing table is visible in the log. 2. **Architecture doc (MEDIUM, Docs).** The architecture doc still shows a stale `restoreCommit` signature and does not mention that the multi-table wrapper forwards it. Since this PR is what makes the hook reachable under Zeta, please correct the signature and add a sentence on the wrapper's forwarding behaviour in the same PR. **Nice to have / can be a follow-up if you prefer, but please say so** 3. **Release/upgrade note (LOW, Docs).** Restore behaviour for JDBC XA and Iceberg under Zeta changes with this PR. Please add a note, and fix the existing JDBC XA entry in `incompatible-changes.md`, which currently describes behaviour that was not actually reachable before this change. 4. **Iceberg restore path (LOW, Functional).** This override also activates `IcebergAggregatedCommitter.restoreCommit` under Zeta for the first time, while the PR only validated JDBC XA. Could you run the existing Iceberg savepoint IT locally against this branch and report the result here? That's enough for me if it passes. 5. **Unit test coverage (LOW, Test).** The new test uses a single table whose `restoreCommit` returns an empty list. Please add one case with two or more tables where at least one returns a non-empty retry list, so the per-table split and the positional retry-list merge on the restore path are actually exercised. 6. **Style (LOW).** Replace the `//` comment above the overriding method with Javadoc (consistent with the rest of this package), and change the raw `List` on the changed line to `List<?>`. If you'd rather defer 3–6, just note that in a reply and I'll track them separately; 1 and 2 I'd like in this PR. Thanks for the focused fix. <!-- streview-comment:2076 --> -- 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]
