DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5340775131
Thanks for the ping, @davidzollo, and for re-checking the restore
reconciliation.
This is a re-verification of the current head,
`17234b67e1d5574f116358f7b8f3b74a862ce496`. I want to be precise about what
actually changed since my last review,
`03f7fd4347f7870d494b8e2babb2f362e947ec74` (2026-08-19, a few hours ago),
because it matters for what this comment is and isn't:
```
git diff 03f7fd4347f7..17234b67e1d5 --stat
```
returns **empty**. The only commit on top of `03f7fd4347f7` is `17234b67e1d`
itself, `[Chore] Retrigger CI for PR #11569` — an empty commit with no file
changes. So this is not a new code version; it is the exact same diff I already
did a full from-scratch re-trace against this morning. I'm not going to pretend
to "discover" things again on identical code — I'll restate my open conclusion
precisely and respond to the new activity since then.
# What Problem Does This PR Solve?
- User pain point: in the JDBC XA (2PC) aggregated-commit path, a permanent
XA commit failure could be silently swallowed (`throwIfAnyFailed("commit")` was
commented out, and `TransientXaException` was double-wrapped so its catch
clause was unreachable dead code). A checkpoint could be reported successful
while the transaction stayed prepared-but-uncommitted in the database.
- Fix approach: reclassify `XA_RBTRANSIENT` -> `XA_RETRY`, make
`wrapException` return instead of throw, re-enable `throwIfAnyFailed`, and
reconcile checkpoint-restored XIDs against a fresh `xaFacade.recover()` scan
using a commit-order (prefix/suffix) invariant rather than a blanket
`XAER_NOTA` tolerance.
- One-sentence summary: this turns a silent "reports success, loses the
commit" bug into a fail-closed commit/restore path — but the current
fail-closed branch for "no evidence either way" is itself unsound, which is
Issue 1 below.
# 1. Code Change Review
## 1.1 Core Logic Analysis — status of my own Issue 1
Re-read the code in place on the current head to confirm nothing moved:
`JdbcSinkAggregatedCommitter.java:193-215` (`replayRecoveredCheckpoint`):
```java
int firstRecoveredIndex = findFirstRecoveredIndex(checkpointXids,
recoveredXids);
if (firstRecoveredIndex < 0) {
throw new JdbcConnectorException(
CommonErrorCodeDeprecated.WRITER_OPERATION_FAILED,
String.format(
"none of the restored checkpoint transactions are
present in the XA recovery scan: %s",
checkpointXids));
}
```
Unchanged from what I traced this morning.
`testRestoreCommitFailsClosedWhenRecoveryScanHasNoCheckpointXid`
(`JdbcSinkAggregatedCommitterTest.java:144-164`) still asserts exactly this
throw, so the behavior is intentional and still covered — it's just the wrong
outcome for the case it covers.
The problem, restated once more for the record since it's the actual
blocker: `CheckpointCoordinator` sets `latestCompletedCheckpoint` as the
durable restore point *before* the phase-2 XA commit for that checkpoint even
runs (`CheckpointCoordinator.java:1330` precedes the `notifyCompleted()` call
at `:1335`), and it stays pointed at that checkpoint until the *next*
checkpoint independently completes. Any restart in that window — for a reason
having nothing to do with XA at all (an unrelated source/task failure, a node
loss) — replays a `restoreCommit()` call whose XidInfo list has, by then,
already been fully and successfully committed by a prior, healthy
`notifyCheckpointComplete()`. Every XID in that batch is legitimately absent
from the resource manager (successfully committed and forgotten, standard XA
behavior), so `findFirstRecoveredIndex` returns `-1` for the *entire* batch —
and the code now treats total absence as proof of an unrecoverable problem
rather than the si
ngle most likely, most benign explanation. Because the RM state driving this
decision doesn't change between restart attempts, the job is stuck permanently
until an operator manually intervenes.
This is a real regression relative to every earlier version of this PR I
reviewed, including the one @nzw921rx and @davidzollo raised CHANGES_REQUESTED
against: the pre-`dd143c0445c6` code tolerated every absent XID
unconditionally, which correctly handled the "whole batch already committed"
case (at the cost of the narrower data-loss risk both of you originally, and
correctly, flagged). The current code closes that data-loss risk but, in doing
so, converts the single most common healthy-restart case into a hard failure
loop. That's a worse outcome for availability than the original bug was for
correctness, and it doesn't require reverting the good parts of this rework —
see the fix direction in Issue 1 below.
## 1.2 Compatibility Impact
**Classification: Fully compatible at the API/config/serialization level**,
unchanged from my prior assessment — no public API break, no config key/default
change, no checkpoint state schema change (`XidInfo`/`JdbcAggregatedCommitInfo`
untouched). The intended behavioral tightening (restore can now fail rather
than silently guess) is documented in `incompatible-changes.md` in both
`docs/en` and `docs/zh`. That documentation is accurate for the code as
written, but the code it describes has the Issue 1 bug, so the compatibility
note currently undersells the operational risk for the all-absent case
specifically.
## 1.3 Performance / Side-Effect Analysis
Unchanged from this morning: `recoverCheckpointTransactions()` adds one
bounded-retry `xaFacade.recover()` scan on restore only (not steady-state
commit); `commitXidInfos`'s round cap has no backoff between rounds (Issue 2,
carryover, still open) — `XAER_RMFAIL`-class outages will still burn the whole
`max_commit_attempts` budget in microseconds.
## 1.4 Error Handling and Logging
Same two open items as this morning's review, both carryovers, neither new:
- **Issue 1 (Blocking):** the `firstRecoveredIndex < 0` branch (above).
- **Issue 2 (Medium):** no backoff between synchronous retry rounds
(`JdbcSinkAggregatedCommitter.java:124-142`, `:234-255`).
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged: Javadoc on all new/changed methods matches their bodies, ASF
headers present, no wildcard imports, no `System.out.println`, AOSP formatting
consistent with the module.
## 2.2 Test Coverage and Test Stability
Test content is byte-for-byte identical to what I reviewed this morning
(confirmed via the empty `git diff --stat`). **Stability rating: Stable** — all
tests remain Mockito-based, single-threaded, deterministic on object
identity/counts/exception message content; no `Thread.sleep`, no shared static
state, no `@DisplayName` usage.
**Gap (Issue 3, Medium, carryover, still open):** there is still no test
where the *entire* checkpoint batch is legitimately, successfully
already-committed (a prior healthy `notifyCheckpointComplete()`, followed by an
unrelated restart) — as opposed to a prior *aborted* restore attempt. That's
the exact scenario Issue 1 describes, and it should be the first regression
test added alongside the fix. `XaGroupOpsImplIT` remains `@Disabled`; there is
still no real-database exercise of any part of this PR's XA commit/restore path.
## 2.3 Documentation Updates
`docs/en/connectors/sink/Jdbc.md`, `docs/zh/connectors/sink/Jdbc.md`, and
both `incompatible-changes.md` files document the restore-order/fail-closed
behavior consistently in both languages. As in 1.2, this is accurate
documentation of code that currently has the Issue 1 bug, so it should be
revisited once that's fixed rather than treated as a separate doc task.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
The commit-order/prefix-suffix reconciliation is a genuinely better idea
than either the original strict-everywhere or tolerant-everywhere positions
from the two earlier CHANGES_REQUESTED reviews — it grounds the tolerance
decision in a real invariant of this code's own sequential commit loop instead
of an unprovable assumption about resource-manager history. The remaining
problem is narrow: the boundary condition where the model has *zero*
still-prepared evidence to anchor the prefix/suffix split against. Fixing that
doesn't require touching the rest of the design.
## 3.2 Maintainability
Good: the responsibility split (`XaFacadeImplAutoLoad` classifies driver
outcomes, `XaGroupOpsImpl` owns grouped propagation,
`JdbcSinkAggregatedCommitter` owns restore reconciliation) is clean, and the
helper methods (`replayRecoveredCheckpoint`, `findFirstRecoveredIndex`,
`recoverCheckpointTransactions`) are small and single-purpose.
## 3.3 Extensibility
Unchanged: `XidKey` is a solid reusable canonical-comparison primitive; the
`default` method on `XaGroupOps.commit(...)` lets other implementors opt in
incrementally without a forced signature change.
## 3.4 Historical-Version Compatibility
No checkpoint-state schema change; old checkpoints/savepoints deserialize
unmodified under this code. Issue 1 is a decision-logic problem, not a
serialization/format compatibility problem, and it affects every upgrade path
equally regardless of checkpoint age.
# 4. Issue Summary
| # | Issue | Location | Severity | Status |
|---|-------|----------|----------|--------|
| 1 | All-absent restore batch throws unconditionally, misclassifying
"already fully committed by a prior healthy checkpoint, unrelated restart" (the
common case) as unrecoverable data loss; recurs on every restart since the
underlying RM state never changes, wedging the job permanently |
`JdbcSinkAggregatedCommitter.java:198-205` | High (Blocking) | Open, carried
forward unchanged from my 2026-08-19T04:31 review; code is byte-identical |
| 2 | No backoff between synchronous commit/recover retry rounds;
`max_commit_attempts` is largely inert against `XAER_RMFAIL`-class outages |
`JdbcSinkAggregatedCommitter.java:124-142`, `:234-255` | Medium | Open,
carryover from 2026-08-16 |
| 3 | No end-to-end or real-database test exercises commit/restore failure
propagating through to checkpoint/job failure, and specifically nothing
exercises "whole batch already committed by a prior successful commit";
`XaGroupOpsImplIT` remains `@Disabled` | test classes; `XaGroupOpsImplIT.java`
| Medium | Open, carryover from 2026-08-16 |
| 4 | CI on the current head (`17234b67e1d`) is a plain retrigger with no
functional diff; the fork's `Build` run for this exact SHA is still `queued` as
of this comment. The prior run against identical code (`03f7fd4347f7`, fork run
`32162717118`) was green on every JDBC/XA-relevant job (unit-test,
updated-modules-integration-test parts 1-8); its only failure was an unrelated
Paimon E2E flake | fork run `32235473661` (queued); prior identical-code run
`32162717118` | Low (process, not source) | Not yet resolved (run pending) |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
1. **Blockers — must be fixed**
- **Issue 1 (High):** the all-absent branch in
`replayRecoveredCheckpoint` must stop treating "no evidence either way" as
proof of failure. Concretely: `firstRecoveredIndex < 0` should log at WARN and
return (treating the whole batch as already resolved), not throw — this is the
single most likely, most benign explanation for total absence, unlike the
partial-absence "gap after a still-prepared XID" branch at `:209-215`, which is
correctly kept as fail-closed because that pattern cannot arise from a
legitimate sequential commit. This is a one-branch fix plus one new regression
test ("all-XIDs-already-committed restore batch completes without error"), not
a redesign — the surrounding commit-order/prefix-suffix machinery is sound and
should be kept exactly as-is.
2. **Recommended fixes — non-blocking**
- Issue 2 (Medium): add bounded backoff between synchronous retry rounds.
- Issue 3 (Medium): add a committer-level test exercising the "whole
batch already committed" restore scenario, and re-enable `XaGroupOpsImplIT`
with real XA coverage.
- Issue 4 (Low): let the pending fork CI run for `17234b67e1d` complete;
no action expected beyond confirming it matches the already-green result for
the identical `03f7fd4347f7` code.
**On @davidzollo's request to re-check and clear the stale review gate:**
the restore reconciliation you asked reviewers to re-check does correctly close
both your 2026-08-06 idempotency finding and @nzw921rx's 2026-07-27
normalization finding — I independently re-verified that again on this pass
(see 3.1). But it isn't ready to clear yet, because I found a new, more severe
issue in the same reconciliation logic on 2026-08-19 (Issue 1 above), a few
hours before your comment. That one hasn't been addressed by any commit since —
the only thing that landed after your comment was the CI-retrigger no-op. So
`reviewDecision=CHANGES_REQUESTED` should stay in place, now anchored to Issue
1 rather than to either of the original two findings.
**Overall assessment:** the underlying engineering direction is sound and
has meaningfully improved across every round of review on this PR. What's
blocking merge right now is one specific branch's failure-mode choice, not the
architecture around it. Once `firstRecoveredIndex < 0` returns instead of
throwing and that's backed by a regression test, I would expect to be able to
approve this on the next pass.
Since this is my own PR, this is posted as a comment rather than an approval.
--
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]
