DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5342836639
This is a re-verification of the current head,
`fa400efc26840cf2ae12eef437c7c6c4bd55cd2d`, one commit past what I reviewed a
few hours ago (`17234b67e1d5`, itself a no-op CI retrigger on top of
`03f7fd4347f7`). This new commit, `[Fix][Connector-V2] Treat fully absent XA
restore batches as resolved`, is a direct response to Issue 1 from my last
review. I re-traced it from scratch against the actual source rather than
assuming the fix is correct because the commit title matches what I asked for.
# 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 dead code). A checkpoint could report success while a transaction
stayed prepared-but-uncommitted in the database.
- Fix approach: reclassify `XA_RETRY`/`XAER_RMFAIL` as transient (not
`XA_RBTRANSIENT`, which is a permanent already-rolled-back outcome), stop
swallowing permanent commit failures, and reconcile checkpoint-restored XIDs
against a fresh `xaFacade.recover()` scan using commit order as evidence
instead of 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, and this latest commit
fixes the one case where "fail-closed" had itself become a permanent
availability regression (a fully-committed batch surviving an unrelated
restart).
# 1. Code Change Review
## 1.1 Core Logic Analysis
The changed method is
`JdbcSinkAggregatedCommitter.replayRecoveredCheckpoint`
(`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/sink/JdbcSinkAggregatedCommitter.java:194-226`),
called from `restoreCommit` once per `JdbcAggregatedCommitInfo` on job/task
restore.
Before (what I flagged as Issue 1):
```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));
}
```
After:
```java
int firstRecoveredIndex = findFirstRecoveredIndex(checkpointXids,
recoveredXids);
if (firstRecoveredIndex < 0) {
log.warn(
"Skipping checkpoint batch because none of its transactions
remain in the XA recovery scan; treating it as already resolved: {}",
checkpointXids);
return;
}
```
The rest of the method — the "gap after a still-prepared XID fails closed"
branch at `:206-217` — is untouched.
Key findings:
- Normal path reached: yes. `restoreCommit` is on Zeta's task-restore
critical path — it runs whenever a JDBC XA sink task restarts from a
checkpoint, which includes both real failure recovery and any unrelated restart
(source/task failure elsewhere, node loss) that happens to fall in the window
between a checkpoint's XA commit completing and the next checkpoint
independently completing.
- Scenario covered: the specific scenario I raised as Issue 1 —
`CheckpointCoordinator` advances `latestCompletedCheckpoint` before phase-2 XA
commit for that checkpoint even runs, and holds it there until the next
checkpoint completes on its own. Any restart in that window replays
`restoreCommit()` against a `XidInfo` batch that a prior, healthy
`notifyCheckpointComplete()` already fully committed. Every XID in that batch
is then legitimately absent from the RM (committed and forgotten — standard XA
behavior), so the old code's unconditional throw converted the single most
common healthy-restart case into a permanent failure loop with no operator
remedy (the RM state doesn't change between retries).
- Precise fix, not a workaround: it targets exactly the boundary condition I
identified — total absence, where the model has zero still-prepared evidence to
anchor the prefix/suffix invariant against — and leaves the rest of the
commit-order reconciliation (which correctly stayed fail-closed for the "gap
after a still-prepared XID" case, since that pattern cannot arise from a
legitimate sequential commit) completely alone. One branch, one behavior change.
- Remaining hole (not new, inherent to XA, already priced into my prior
review): treating total absence as "already resolved" is still a heuristic.
XAER_NOTA-equivalent absence at scan time cannot distinguish "committed and
forgotten" (the common, targeted case) from "rolled back/expired/heuristically
completed for an unrelated reason and then discarded by the RM." A fully
rigorous fix needs an external durable record of "we ourselves issued a
successful commit," independent of RM bookkeeping — out of scope for a
one-branch fix. Given the WARN log at the point of tolerance and that this
narrows rather than widens the pre-existing ambiguity (the pre-PR code
tolerated absence unconditionally in every case, not just this one), I'm not
re-opening this as a new blocker; it's the same class of residual risk every
version of this PR has carried, not something this commit makes worse.
- Runtime path (restore, current head):
```text
Task restart / restore
-> JdbcSinkAggregatedCommitter.restoreCommit(aggregatedCommitInfos)
[L91-99]
-> tryOpen()
-> for each JdbcAggregatedCommitInfo:
-> recoverCheckpointTransactions() [L234-255]
-> xaFacade.recover(), bounded retry on TransientXaException
-> replayRecoveredCheckpoint(checkpointXids, recoveredXids)
[L194-226]
-> findFirstRecoveredIndex() [L264-271]
-> branch: no checkpoint XID found in scan
(firstRecoveredIndex < 0)
-> log.warn(...), return [FIXED: was throw]
-> branch: some XID still prepared
-> verify every XID from firstRecoveredIndex onward is
present
-> absent -> throw (unchanged, fail-closed, correct)
-> commitXidInfos(stillPrepared, ignoreUnknown=false)
[L218]
-> log.warn for the already-resolved prefix, if any
[L219-225]
```
## 1.2 Compatibility Impact
**Classification: Fully compatible.** No API/config/serialization change —
`XidInfo`/`JdbcAggregatedCommitInfo` untouched, no new/renamed config option.
The behavioral change is a narrowing of the fail-closed surface introduced
earlier in this same PR (not yet released), so there's no compatibility break
relative to any shipped version. `docs/en` and `docs/zh` (`Jdbc.md` and
`incompatible-changes.md`) were updated in the same commit and now accurately
describe "all-absent -> treated as already resolved, skip" vs. "gap after a
still-prepared XID -> fail closed" — I checked both language versions and they
say the same thing.
## 1.3 Performance / Side-Effect Analysis
No change in this commit. Carryover from my last pass, still true:
`recoverCheckpointTransactions()` runs once per `JdbcAggregatedCommitInfo`
inside `restoreCommit`'s loop rather than once per invocation — a pre-existing
minor inefficiency (extra RM round-trips when a restore batch has multiple
`JdbcAggregatedCommitInfo` entries), not a correctness issue, and not something
this commit touches. `commitXidInfos`'s round cap still has no backoff between
rounds (Issue 2 below).
## 1.4 Error Handling and Logging
The fix moves this branch from "throw, opaque to a human reading the
checkpoint failure without XA context" to "WARN with the full XID list, then
continue" — this is strictly better for both correctness (no more permanent
wedge) and operability (the WARN gives an operator something to grep for if
they want to double check RM history). I verified `XaFacadeImplAutoLoad`'s
error classification independently (not just re-reading the prior review):
`TRANSIENT_ERR_CODES = {XA_RETRY, XAER_RMFAIL}`
(`XaFacadeImplAutoLoad.java:75-76`) correctly excludes `XA_RBTRANSIENT` (a
permanent already-rolled-back outcome per the XA spec, not a "no effect, retry
me" outcome), and `buildCommitErrorDesc` (`:323-331`) only tolerates
`XAER_NOTA` when `ignoreUnknown=true` — every other code, including the
`XA_HEURRB`/rollback family, still propagates as a hard failure. This confirms
the classification correctness I'd only taken on trust before.
**Issue 2 (Medium, carryover, unchanged by this commit):** no backoff
between synchronous commit/recover retry rounds —
`JdbcSinkAggregatedCommitter.java:129-142` (`commitXidInfos`) and `:237-248`
(`recoverCheckpointTransactions`). `XAER_RMFAIL`-class outages will burn the
whole `max_commit_attempts` budget in microseconds with no delay between
attempts.
# 2. Code Quality Assessment
## 2.1 Coding Standards
The updated Javadoc on `restoreCommit` (`:84-89`) matches the new behavior
("An all-absent batch is treated as already resolved..."). No missing
documentation on the changed method.
## 2.2 Test Coverage and Test Stability
`JdbcSinkAggregatedCommitterTest.testRestoreCommitSkipsAlreadyResolvedBatchWhenRecoveryScanHasNoCheckpointXid`
(`:143-163`) was rewritten in the same commit: it now uses a 2-XID all-absent
batch, asserts `assertDoesNotThrow`, and verifies `xaGroupOps.commit(...)` is
`never()` invoked — which matches the implementation exactly (the method
returns before building the `stillPrepared` list, so commit is genuinely never
attempted, not just swallowed). This closes the exact gap I called Issue 3 last
time ("no test where the entire batch is legitimately already-committed") at
the unit level.
**Stability rating: Stable.** Mockito-based, single-threaded, deterministic
on object identity/counts/message content; no `Thread.sleep`, no shared static
state, no `@DisplayName`.
**Issue 3 (Medium, carryover, partially addressed):** the unit-level gap is
now closed, but there is still no integration/real-database exercise of any
part of this PR's XA commit/restore path — `XaGroupOpsImplIT`
(`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/xa/XaGroupOpsImplIT.java`)
remains `@Disabled` and untouched by this PR (confirmed: `git diff dev...HEAD`
for that path is empty).
## 2.3 Documentation Updates
`docs/en/connectors/sink/Jdbc.md`, `docs/zh/connectors/sink/Jdbc.md`, and
both `incompatible-changes.md` files were updated in this same commit to
describe the corrected all-absent behavior. Consistent between languages; I
read both.
# 3. Architectural Soundness
## 3.1 Elegance
Precise fix. One branch changed from throw to warn-and-return, with an
updated regression test and updated docs in the same commit — no redesign, no
scope creep into the (still correctly fail-closed) gap-after-recovered branch.
## 3.2 Maintainability
Unchanged from my last assessment: the responsibility split across
`XaFacadeImplAutoLoad` (driver-outcome classification), `XaGroupOpsImpl`
(grouped commit propagation), and `JdbcSinkAggregatedCommitter` (restore
reconciliation) stays clean;
`replayRecoveredCheckpoint`/`findFirstRecoveredIndex`/`recoverCheckpointTransactions`
remain small and single-purpose.
## 3.3 Extensibility
Unchanged: `XidKey`'s canonical `formatId`+GTRID+BQUAL comparison is a
solid, reusable primitive; `XaGroupOps.commit(...)`'s new `default` overload
lets other implementors opt in without a forced signature break.
## 3.4 Historical-Version Compatibility
No checkpoint-state schema change; old checkpoints/savepoints deserialize
unmodified. This fix is a decision-logic change, not a serialization/format
change, and applies uniformly regardless of checkpoint age.
# 4. Issue Summary
| # | Issue | Location | Severity | Status |
|---|-------|----------|----------|--------|
| 1 | ~~All-absent restore batch threw unconditionally, misclassifying
"already fully committed by a prior healthy checkpoint, unrelated restart" as
unrecoverable, wedging the job permanently~~ |
`JdbcSinkAggregatedCommitter.java:198-205` | High | **Fixed in `fa400efc2`,
verified** |
| 2 | No backoff between synchronous commit/recover retry rounds;
`max_commit_attempts` is largely inert against `XAER_RMFAIL`-class outages |
`JdbcSinkAggregatedCommitter.java:129-142`, `:237-248` | Medium | Open,
carryover |
| 3 | No end-to-end/real-database test exercises commit/restore failure
propagating to checkpoint/job failure; `XaGroupOpsImplIT` remains `@Disabled`
and untouched by this PR | `XaGroupOpsImplIT.java` (connector-jdbc-e2e-part-1)
| Medium | Open, carryover (unit-level gap for the specific all-absent scenario
is now closed) |
| 4 | Fork `Build` run for the current head (`fa400efc2`) was still in
progress at review time: all completed jobs green, including every `unit-test`
lane (which runs this PR's own new/changed tests), zero failures observed
across 56 completed jobs; ~15 `updated-modules-integration-test-part-*` jobs
still running | fork run `32248144048` (in progress) | Low (process, not
source) | Not yet finalized |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed**
- None from source review. Issue 1, the sole High/blocking finding from
my prior pass, is fixed and independently re-verified against the current head.
The only open item before this can be called done is Issue 4: let the
in-progress fork CI run for `fa400efc2` finish and confirm it matches the
all-green result already observed for every completed job (especially the
`unit-test` lanes, which already cover this exact fix on this exact SHA).
2. **Recommended fixes — non-blocking**
- Issue 2 (Medium): add bounded backoff between synchronous retry rounds.
- Issue 3 (Medium): re-enable `XaGroupOpsImplIT` with real XA coverage,
including the "whole batch already committed by a prior successful commit, then
an unrelated restart" scenario at the integration level (the unit test now
covers this at the mock level).
**On the reconciliation logic overall:** I re-confirm what I found in my
morning pass — the commit-order/prefix-suffix invariant correctly closes both
@nzw921rx's 2026-07-27 normalization concern and @davidzollo's 2026-08-06
idempotency concern, and this commit closes the one new problem I found on top
of that (Issue 1). No new problem introduced by this commit; the change is
narrowly scoped to exactly the branch I asked about, with a matching regression
test and matching doc update in the same commit.
**Overall assessment:** the engineering direction has been sound throughout
this PR's review history, and every round has converged rather than regressed.
This round closes the last blocking item I had open. Once the current fork CI
run finishes green, I'd expect to be able to call this ready to merge without
qualification.
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]
