DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5462160229
Note: this PR is authored by me (DanielLeens), so GitHub blocks a
self-review submission. Posting this as a plain issue comment instead of a
formal PR review, per project convention for self-authored PRs (same as my
previous rounds on this thread).
I re-checked activity since my last comment (`a9257e75bf1a`, ~10 hours ago):
`git diff` between that head and the current head is empty — no new commits, no
new reviews, no new discussion. Given that, I'm not going to re-post the
~2000-word from-scratch trace verbatim again; it would add nothing over what's
already sitting immediately above this comment on the same code. What follows
is condensed to (a) an independent spot-check of my own prior conclusions using
a different method than before, and (b) one genuinely new finding that none of
the six review rounds on this thread (mine included) named explicitly.
# What Problem Does This PR Solve?
Unchanged from prior rounds: before this patch, a permanent JDBC XA commit
failure could be silently swallowed (`throwIfAnyFailed("commit")` was disabled,
and `wrapException()` always threw `TransientXaException` pre-wrapped inside
`JdbcConnectorException`, making the retryable-error branch unreachable). This
PR restores failure propagation, fixes retryable/permanent XA error
classification, bounds retries within one invocation, and reconciles restored
checkpoint XIDs against a live `xaFacade.recover()` scan instead of inferring
success from an absent XID alone.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Independent verification of the failure-propagation fix**
(`XaFacadeImplAutoLoad.java`, `XaGroupOpsImpl.java`): confirmed by reading
every `wrapException` call site, not just the two I discussed before. All four
sites (`execute()`'s `orElseThrow`, `execute()`'s catch-all `throw`, and both
`Command.fromRunnable`/`fromRunnableRecoverByWarn` closures) consistently do
`throw wrapException(...)`. Since `wrapException` now *returns* a
`RuntimeException` (either `TransientXaException` or `JdbcConnectorException`)
instead of throwing it pre-wrapped, every one of those four call sites now
throws the correct concrete type, so `XaGroupOpsImpl.commit()`'s `catch
(XaFacade.TransientXaException e)` (previously dead code, since the exception
used to arrive as a `JdbcConnectorException` wrapping a `TransientXaException`
inside its cause, not as a `TransientXaException` itself) is genuinely
reachable. `TRANSIENT_ERR_CODES` correctly moved from `{XA_RBTRANSIENT}` to
`{XA_RETRY, XAER_RMFAIL}` —
`XA_RB*` codes mean the branch is already rolled back (permanent, nothing to
retry), `XA_RETRY` means the call had no effect and is safe to reissue. This is
real XA semantics, not a cosmetic rename.
**Independent verification of the ordering invariant** the restore
reconciliation depends on: `XaGroupOpsImpl.commit()`'s loop condition is
`i.hasNext() && (result.hasNoFailures() || allowOutOfOrderCommits)`, both
production call sites pass `allowOutOfOrderCommits=false`, and
`hasNoFailures()` flips to `false` on *either* a permanent or a transient
failure. So within one invocation the loop stops at the first failure of any
kind, and every subsequent un-iterated XID is appended to `forRetry` untouched
(`result.getForRetry().addAll(xids)` after the loop, using the same `Iterator`,
so relative order is preserved). Across retry rounds in `commitXidInfos`'s
`while (!pending.isEmpty())` loop, `pending` is rebuilt from
`result.getForRetry()` each round, so order is preserved across rounds too.
This confirms the property
`replayRecoveredCheckpoint`/`findFirstRecoveredIndex`
(`JdbcSinkAggregatedCommitter.java:205-237`) relies on: a later XID in a
checkpoint's list is only ever committed a
fter every earlier XID in that same list already committed. I traced this
end-to-end myself rather than trusting the Javadoc's claim, and it holds on the
current head.
**New finding — Issue 1 below.** Tracing the two "absorb an absence"
branches (all-absent batch, and the skipped prefix before a still-prepared
suffix) surfaced a gap that none of the six review rounds on this PR (nzw921rx,
davidzollo, li3zhi4, JeremyXin, dybyte, or my own five prior rounds) named
explicitly, though dybyte's Q1 thread on `JdbcSinkAggregatedCommitter.java:103`
came close from a different angle (concurrent resolution timing, which I
answered fully for the still-prepared/suffix case in that thread — external
resolution there surfaces as a loud `XAER_NOTA`-is-permanent failure, since
`XAER_NOTA` isn't in `TRANSIENT_ERR_CODES`). What I hadn't examined is the
*prefix/all-absent* case specifically:
### Issue 1: Absence from the recovery scan cannot distinguish "committed by
us" from "rolled back by someone else," and only the fail-closed (gap) case is
disclosed to operators
- **Location**: `JdbcSinkAggregatedCommitter.java:205-237`
(`replayRecoveredCheckpoint`), and the corresponding entries in
`docs/en(zh)/introduction/concepts/incompatible-changes.md` /
`docs/en(zh)/connectors/sink/Jdbc.md`
- **Problem**: Per the XA spec, a resource manager forgets a branch
identically whether it was previously *committed* or *rolled back* — both
outcomes make the branch vanish from `recover()`, and both return `XAER_NOTA`
if a client tries to act on it again. The prefix-skip and all-absent-batch
branches use "absent from the scan" as their *only* signal, and both
optimistically assume "already committed by us in a prior aborted attempt."
Under this PR's own architecture that assumption is well-justified for the
*normal* failure mode it fixes (a batch's own earlier commit attempt got
interrupted by an unrelated restart) — but as dybyte's Q1 thread already
established, the resource manager can also be resolved by an actor external to
Zeta: a DBA or an automated stuck-prepared-transaction cleanup job. If that
external actor issues `XA ROLLBACK` instead of `XA COMMIT` against a branch
that actually still belongs to a live (merely delayed) SeaTunnel job — a
realistic scenario after
a long outage, since long-pending prepared XA transactions are a well-known
operational headache that provokes exactly this kind of manual/automated
cleanup — this code has no way to tell the difference, and unlike the
suffix/gap case, the prefix/all-absent branches never re-attempt a commit on
the skipped XIDs, so there is no `XAER_NOTA`-permanent-failure signal to
surface the mistake. The checkpoint is reported as fully resolved while that
XID's writes are permanently gone. The `log.warn` at
`JdbcSinkAggregatedCommitter.java:212-214` and `:232-235` is the only
operational trace, and it reads identically whether the skip was correct (the
very common case) or masking a real loss (the rare case) — an operator cannot
tell which happened from the log alone.
- **Why this isn't a reason to reject the design**: the alternative —
failing closed on every absence, including the all-absent case — is exactly the
infinite-restart-deadlock bug davidzollo's 2026-08-06 review found in an
earlier revision of this PR (a job stuck forever re-attempting a batch that was
legitimately already committed). XA fundamentally cannot distinguish "resolved
by commit" from "resolved by rollback" from the RM's response alone without
extra durable state that Zeta doesn't checkpoint today (the PR description is
upfront about this constraint), so some assumption is unavoidable, and "assume
committed" is the correct default given SeaTunnel's own aborts never touch a
checkpoint-committed XID's namespace (each checkpoint/subtask generates
distinct XIDs, so self-inflicted confusion is ruled out — I checked this
specifically since it would have been a much worse bug).
- **Potential risk**: silent, permanent, undetectable loss of one XID's
writes in the narrow window where an external actor rolls back (rather than
commits) a prepared transaction this job still expects to restore — with no
distinguishing signal from a routine, correct skip.
- **Best improvement**: this doesn't need a code change (there's no code fix
for an information-theoretic gap), but the incompatible-changes migration guide
currently only tells operators to investigate a *gap after a recovered XID*
before upgrading — it should say the same for the all-absent/prefix-skip case:
that "already resolved" there is inferred, not verified, and that operators who
run external stuck-transaction cleanup against a resource manager also used by
SeaTunnel should exclude branches whose global-transaction-id prefix matches
SeaTunnel's `XidGenerator` format, or otherwise coordinate with job restart
timing. A one-line addition to the existing migration guide entry closes this
cheaply.
- **Severity**: Medium (real residual risk, correctly scoped as
inherent-to-XA rather than a implementation bug; recommended before merge, not
a hard blocker given the log-level signal that does exist and the fact this PR
is a strict improvement over the pre-existing status quo either way)
- **Raised by another reviewer**: No — new angle on a concern dybyte's Q1
partially opened; my own reply in that thread fully closed the
suffix/still-prepared case but did not address the prefix/all-absent case,
which is the part I'm completing here.
## 1.2 Compatibility Impact
Unchanged from prior rounds: Partially incompatible, and correctly disclosed
for the gap/fail-closed scenario (see Issue 1 for the one additional disclosure
gap). No checkpoint state schema changed (`XidInfo` serialization untouched);
`TransientXaException`'s constructor widened from package-private to `public`;
`XaGroupOps.commit()` gained Javadoc only. No source-incompatible API changes.
## 1.3 Performance / Side-Effect Analysis
Unchanged: `restoreCommit()`'s `recover()` scan is a bounded,
restart-only-path cost with its own retry budget; the 1s `Thread.sleep` backoff
between synchronous retry rounds (added in response to JeremyXin's 2026-08-21
review) only fires under genuine XA/RM instability; `containsEquivalentXid` is
an O(N) scan per checkpoint XID against a `Set<XidKey>`, bounded by batch size.
## 1.4 Error Handling and Logging
No new issues beyond Issue 1 above. I independently re-verified
`throwIfAnyReachedMaxAttempts` in `XaGroupOpsImpl.java` and `commitXidInfos`'s
outer bounded loop in `JdbcSinkAggregatedCommitter.java` both actually
terminate (covered by `testCommitFailsWhenRetryRoundsNeverDrain`) rather than
looping forever on a permanently-stuck resource manager.
**CI diagnosis for the current head (`a9257e75bf1a`)** — unchanged from my
comment immediately above, and I independently re-verified it rather than
taking my own prior word for it: pulled the actual `unit-test (8,
ubuntu-latest)` job log from the fork run (`DanielLeens/seatunnel` run
`33238169880`, job `99063544383`) myself. Confirmed the failure is exactly
`[ERROR]
CoordinatorServiceTest.testClearCoordinatorServiceDropsPendingJobsUnderRejectStrategy:642
» ConditionTimeout`, in `seatunnel-engine-server`, a module this PR's diff
never touches (this PR's diff is scoped to `connector-jdbc`'s XA classes/tests,
`connector-jdbc-e2e-part-1`, and docs — confirmed via `git diff --stat` against
the correct merge-base `43fe63b1fca`). The other three failing jobs
(`updated-modules-integration-test-part-3`/`part-2` on JDK 8 and 11) were
already diagnosed as Maven Central dependency-resolution resets and an
unrelated `PostgresCDCIT` Awaitility timeout, in modules this PR does not touch
ei
ther. This is infra flake, not a regression from this PR — needs a CI rerun,
not a code change.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged: all new/changed core methods carry Javadoc explaining intent and
the fail-closed/already-resolved distinction. No gaps found.
## 2.2 Test Coverage and Test Stability: Stable
Unchanged rating, independently re-confirmed by reading the full method list
in `JdbcSinkAggregatedCommitterTest.java`: dedicated coverage exists for
recovered-by-value matching, per-batch recovery-scan refresh,
prefix-skip-after-recovered-suffix, all-absent-batch skip, fail-closed-on-gap,
transient recovery-scan retry, bounded commit-retry completion, and
retry-exhaustion. `XaGroupOpsImplIT` exercises a real MySQL testcontainer end
to end (addresses JeremyXin's Issue 2). No test exists for the Issue 1 scenario
above, which is expected — there is no deterministic way to unit-test "an
external actor rolled back instead of committed," since the code itself cannot
observe the difference; this is a documentation gap, not a missing-test gap.
## 2.3 Documentation Updates
Current docs accurately describe the mechanism (verified by diff, not just
PR description) but have the one gap named in Issue 1.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution: Precise fix
Unchanged: scoped tightly to the XA commit/restore path, no leakage into
`seatunnel-api` or the engine.
## 3.2 Maintainability
Unchanged: small, single-purpose, documented helpers (`XidKey`,
`backoffBeforeRetry`, the commit/restoreCommit split).
## 3.3 Extensibility
Unchanged: reconciliation operates on canonical XID values via the existing
`XaFacade`/`XaGroupOps` abstractions.
## 3.4 Historical-Version Compatibility
Unchanged: no wire/state-format change; the risk is purely operational and
disclosed for the gap case (Issue 1 extends this disclosure to the
prefix/all-absent case).
# 4. Issue Summary
| Number | Issue | Location | Severity |
| --- | --- | --- | --- |
| 1 | Absent-from-recovery-scan XIDs (prefix skip, all-absent batch) are
assumed committed with no way to distinguish an externally-issued rollback;
only the gap/fail-closed case is disclosed in the migration guide |
`JdbcSinkAggregatedCommitter.java:205-237`,
`docs/en(zh)/introduction/concepts/incompatible-changes.md` | Medium |
| N/A-2 | `XaFacade.commit(xid, ignoreUnknown=true)` has no production
caller after evidence-based replay replaced `XAER_NOTA`-as-success inference
(li3zhi4, still open) | `JdbcSinkAggregatedCommitter.java`,
`XaFacadeImplAutoLoad.java` | Low |
| N/A-3 | PR description overstates the all-absent case as "fails closed"
when code/docs treat it as already-resolved (li3zhi4, still open,
description-only) | PR description text | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge
Issue 1 is a real, previously-unarticulated residual risk, but it's an
inherent consequence of XA's own commit/rollback ambiguity rather than an
implementation defect, it's already partially mitigated by WARN-level logging,
and the fix is a one-line addition to an already-thorough migration guide
rather than a code change — I don't think it should block merge, but I'd like
it addressed before this lands given how much this PR already invests in
disclosing the gap-case risk to operators.
Restating what hasn't changed: both outstanding `CHANGES_REQUESTED` reviews
(nzw921rx 2026-07-27, davidzollo 2026-08-06) target concerns the current
implementation structurally resolves — I re-verified this myself from source
rather than assuming my prior rounds still hold. That's a process blocker, not
a code blocker: I'm the PR author and cannot dismiss another reviewer's review
or approve my own PR; this needs nzw921rx/davidzollo to re-review the current
head, or a maintainer with write access to dismiss the stale reviews. @dybyte's
`APPROVED` (2026-08-28, same head modulo the empty CI-retry commit) is
independent confirmation the current implementation is sound.
1. Blockers (process, not code): the two stale `CHANGES_REQUESTED` reviews
need to be revisited/dismissed by their authors or a maintainer. CI needs a
clean rerun given the four confirmed-unrelated failure causes above
(independently re-verified this round, not just carried forward).
2. Recommended fixes (non-blocking): add the one-line migration-guide
disclosure for Issue 1; the two low-severity carryovers from li3zhi4 (PR
description wording, unused `ignoreUnknown=true` path).
No alternative implementation approach changes here; the
commit-order-evidence design remains the right shape given that Zeta doesn't
persist partial-commit state across a checkpoint restart today.
--
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]
