DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5459138261
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. This is a full
re-review from scratch against the current head, not a diff against my prior
rounds.
# What Problem Does This PR Solve?
Before this patch, a permanent JDBC XA commit failure could be silently
swallowed: `XaGroupOpsImpl.commit()` recorded the failure but never surfaced it
(`throwIfAnyFailed("commit")` was disabled), and `wrapException()` always
wrapped `TransientXaException` inside `JdbcConnectorException`, making the
`catch (TransientXaException)` branch in the group-commit loop unreachable. The
net effect was that every XA commit failure, transient or permanent, could let
a checkpoint be reported successful while the transaction was left
prepared-but-uncommitted in the database. This PR restores failure propagation,
fixes the retryable/permanent error classification, bounds retries within a
single invocation (since Zeta does not persist attempt counters across a
checkpoint restart), and reconciles restored checkpoint XIDs against a live
resource-manager recovery scan instead of inferring success from an absent XID
alone.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Reviewed at head `ca15d97a9ba` (13 commits ahead of `dev` at `43fe63b1fca`,
merge-base is the current `dev` tip). The single commit since my last round
(`403a0b09f88` -> `ca15d97a9ba`, "[Fix][Connector-V2] Clarify XA restore
recovery evidence") is comment-only: it rewords the Javadoc/inline comments on
`restoreCommit()` to state that the per-batch recovery-scan refresh is
intentional (a DBA or RM cleanup process can resolve an in-doubt transaction
between restored batches), addressing li3zhi4's non-blocking Suggestion 3 from
2026-08-21. I confirmed via `git diff 403a0b09f88..ca15d97a9ba` that no
executable code changed.
Because this is a full re-review, I retraced the whole runtime path myself
rather than relying on my own or others' prior conclusions:
**Failure classification and propagation**
(`XaFacadeImplAutoLoad.wrapException`, `XaGroupOpsImpl.commit`):
`TRANSIENT_ERR_CODES` is now `{XA_RETRY, XAER_RMFAIL}` (moved off
`XA_RBTRANSIENT`, which means the branch was already rolled back — a permanent
outcome, not a retryable one). `wrapException` now returns
`TransientXaException` directly instead of throwing it wrapped inside
`JdbcConnectorException`, so `XaGroupOpsImpl.commit()`'s `catch
(XaFacade.TransientXaException e)` branch is reachable, and
`result.throwIfAnyFailed("commit")` (re-enabled) now actually fails the
checkpoint on a permanent/unknown outcome instead of returning a false success.
**Ordering invariant behind the restore design** (this is the load-bearing
correctness property, and I verified it directly rather than taking the PR
description's word for it): `XaGroupOpsImpl.commit()` iterates `xids` with the
guard `i.hasNext() && (result.hasNoFailures() || allowOutOfOrderCommits)`, and
`GroupXaOperationResult.hasNoFailures()` returns false once *either* a
permanent failure *or* a transient failure has been recorded. Both production
call sites pass `allowOutOfOrderCommits = false`. So within one `commit()`
invocation, the loop stops at the *first* failure of any kind, and every
subsequent un-iterated XID is appended to `forRetry` untouched, preserving its
position. Across `commitXidInfos()`'s own retry rounds, `pending` is rebuilt
from `getForRetry()` each round, preserving relative order. This means: a later
XID in the list is committed only if every earlier XID already committed
successfully. That is exactly the invariant `restoreCommit()` ->
`replayRecovered
Checkpoint()` depends on when it treats "everything before the first
still-prepared XID" as already resolved. I traced this end to end and it holds
on the current head.
**Restore reconciliation**
(`JdbcSinkAggregatedCommitter.replayRecoveredCheckpoint`): finds the first
checkpoint XID still present in a fresh `xaFacade.recover()` scan
(`findFirstRecoveredIndex`); everything before it is logged and skipped as
already resolved (justified by the ordering invariant above); everything from
that index onward must *all* still be present in the scan or the batch fails
closed with a `JdbcConnectorException` before anything in that suffix is
committed (no partial commit on the fail-closed path); the still-prepared
suffix is then replayed through the same `commitXidInfos` path used by normal
commit. An all-absent batch (no checkpoint XID at all in the scan) is treated
as already resolved and skipped without ever calling `xaFacade.commit`, which
also means restore no longer needs `XAER_NOTA`-as-success inference at all for
that case.
This directly resolves the two outstanding `CHANGES_REQUESTED` reviews on
this PR:
- **nzw921rx (2026-07-27)** asked for restore to reconcile against
`xaFacade.recover()` instead of trusting `XAER_NOTA` alone, with an explicit,
durable commit-decision rule. That review predates the recovery-scan
reconciliation entirely (it was posted the day after the PR opened, before this
mechanism existed). The current head implements exactly the reconciliation
shape requested: checkpoint XIDs are intersected with the recovery scan, XIDs
not owned by the current batch are never touched (`XidKey`-based canonical
comparison, not driver object identity), and unknown/missing XIDs no longer
fall back to `XAER_NOTA`-is-success.
- **davidzollo (2026-08-06)** found a concrete non-idempotency bug: if
`xidA` commits and `xidB` then permanently fails in the same batch, the success
of `xidA` was previously not recorded anywhere durable, so a restart's
`restoreCommit()` would re-attempt `xidA`, get `XAER_NOTA` (forgotten, since it
was already committed), and treat that as a **new** permanent failure — an
infinite restart loop that never even reaches `xidB`. I reconstructed this
exact scenario against the current code: `findFirstRecoveredIndex` finds `xidB`
(still prepared) at index 1, treats `xidA` (index 0, absent from the scan) as
an already-resolved prefix, and only replays `xidB`. `xidA` is never
recommitted and never reported as a failure. This is precisely the
"commit-order evidence" mechanism the PR description names, and it holds under
the ordering invariant traced above.
**li3zhi4 (2026-08-21, COMMENTED, not blocking)** independently re-derived
both of the above from source at a slightly earlier head (`61df8fd43`,
functionally identical to the current head except for the comment-only commit)
and reached the same conclusion: no correctness blocker, three non-blocking
suggestions. I re-verified all three against the current head:
- Suggestion 1 (PR description overstates the all-absent case as "fails
closed" when the code/docs treat it as already-resolved): still an unaddressed
**PR-description-only** discrepancy; code and docs are internally consistent
and match what li3zhi4 confirmed. Non-blocking, cosmetic.
- Suggestion 2 (`XaFacade.commit(xid, ignoreUnknown)`'s `ignoreUnknown =
true` path has no production caller — both `commitPreparedTransactions` and
`replayRecoveredCheckpoint` call it with `false`): confirmed still true on the
current head; the `true` path is exercised only by unit tests. Non-blocking,
but worth a follow-up: either document the intended production use or narrow
the API now that evidence-based replay has replaced `XAER_NOTA`-as-success
inference.
- Suggestion 3 (clarify why the recovery scan is refreshed per batch instead
of hoisted out of the loop): **addressed** by the one new commit on this head.
## 1.2 Compatibility Impact
Partially incompatible, and correctly disclosed. This is a deliberate,
documented behavior change to the XA exactly-once restore path: jobs that
previously relied on restore silently treating a missing-with-a-gap XID as
success will now fail closed and require operator investigation.
`docs/en/introduction/concepts/incompatible-changes.md` and
`docs/zh/introduction/concepts/incompatible-changes.md` both carry a new entry
describing the affected component, the behavior change, the operational impact,
and migration guidance (inspect `XA RECOVER` / `pg_prepared_xacts` for dangling
prepared transactions before upgrading). No public API signatures changed in a
way that breaks source compatibility; `TransientXaException`'s constructor
visibility widened from package-private to `public`, and `XaGroupOps.commit()`
gained Javadoc only.
## 1.3 Performance / Side-Effect Analysis
- `restoreCommit()` now performs one `xaFacade.recover()` scan per restored
batch instead of zero; this is a bounded, restart-only-path cost, not a
steady-state one, and is retried with the same backoff budget as commit
(`recoverCheckpointTransactions`) so a transient RM outage during restore does
not fail restore immediately.
- A 1-second `Thread.sleep` backoff was added between synchronous retry
rounds in both the commit and recovery-scan retry loops. This only fires during
genuine XA/RM instability (a real net improvement over the pre-PR behavior,
which had effectively zero retries — any non-empty result cancelled the
pipeline on the first transient blip). Two of the new unit tests exercise this
real sleep (bounded to a couple of seconds each), which is an acceptable,
deterministic cost, not a source of flakiness.
- `containsEquivalentXid` is an O(N) linear scan per checkpoint XID against
a `Set<XidKey>`, so the reconciliation as a whole is effectively O(N) per batch
(the recovered side is already a `Set`). This is restore-path-only and bounded
by batch size; not a concern.
## 1.4 Error Handling and Logging
No new formal issues found on this head. All previously raised concerns
(nzw921rx, davidzollo) are, on inspection of the current code, structurally
resolved rather than merely asserted to be resolved.
# 2. Code Quality Assessment
## 2.1 Coding Standards
New/changed core methods (`restoreCommit`, `replayRecoveredCheckpoint`,
`commitXidInfos`, `recoverCheckpointTransactions`, `findFirstRecoveredIndex`,
`containsEquivalentXid`, `backoffBeforeRetry`, `wrapException`, `XidKey`) all
carry Javadoc explaining intent, constraints, and (where relevant) the
fail-closed/already-resolved distinction — this is exactly the kind of "why"
documentation this project's `CLAUDE.md` conventions ask for on complex
conditional/retry logic, and it is genuinely useful here given how easy this
reconciliation logic is to get subtly wrong. No missing-comment gaps found on
this pass.
## 2.2 Test Coverage and Test Stability: Stable
- `XaFacadeImplAutoLoadTest` and `XaGroupOpsImplTest` genuinely simulate XA
failures by `doThrow`-ing real `XAException` error codes (`XAER_RMFAIL`,
`XA_RETRY`, `XA_RBTRANSIENT`, `XAER_NOTA`, `XA_HEURCOM`) from a mocked
`XAResource`/`XaFacade`, not just mocking a success path — this was a specific
concern going into this round and it checks out.
- `JdbcSinkAggregatedCommitterTest` (8 methods) exercises the reconciliation
logic directly: prefix-skip after a still-prepared suffix commits,
all-absent-batch skip, fail-closed on a gap after the first recovered XID,
transient recovery-scan retry, and bounded-retry exhaustion
(`testCommitFailsWhenRetryRoundsNeverDrain` deliberately uses a non-draining
`XaGroupOps` stub to prove the outer loop still terminates and fails rather
than looping forever — a good defense-in-depth test for a retry loop).
- Notably, this PR also fixes a real, unrelated pre-existing
test-infrastructure bug as a side effect: `XaGroupOpsImplIT`
(`seatunnel-e2e/.../connector-jdbc-e2e-part-1`) was `@Disabled` with reason
"JdbcDatabaseContainer: ClassNotFoundException: com.mysql.jdbc.Driver". This PR
removes that `@Disabled` and adds
`testCommitFailurePropagatesThroughAggregatedCommitter`, which prepares a real
XA transaction against a live MySQL Testcontainer, stops the container to force
a genuine RM-unavailable commit failure, and asserts the failure propagates as
a `JdbcConnectorException` through the actual
`JdbcSinkAggregatedCommitter.commit()` path (not a mock). This is real,
non-mocked evidence that the fix works end to end against a real resource
manager, which is a meaningfully stronger signal than the unit tests alone.
- All tests are deterministic (mock-driven or single-container-stop), no
timing-sensitive assertions beyond the bounded, intentional retry-backoff
sleeps noted above.
## 2.3 Documentation Updates
`docs/en/connectors/sink/Jdbc.md` and `docs/zh/connectors/sink/Jdbc.md` both
updated to describe the new restore semantics under
`max_retries`/`max_commit_attempts`;
`docs/en/introduction/concepts/incompatible-changes.md` and the `zh`
counterpart both carry the breaking-change entry with migration guidance.
Content is consistent with the actual code behavior (verified directly against
`replayRecoveredCheckpoint`), not just with the PR description (which, per
li3zhi4's Suggestion 1 above, has one stale sentence that overstates the
all-absent case — worth a quick edit to the PR description text, no doc or code
change needed).
# 3. Architectural Soundness
## 3.1 Elegance of the Solution: Precise fix
The fix is scoped tightly to the XA commit/restore path and does not leak
connector-specific logic into `seatunnel-api` or the engine. The
commit-order-evidence design is a reasonable way to distinguish "already
committed in a prior aborted attempt" from "genuinely lost/unknown" without
persisting additional state across checkpoints, which the PR description is
upfront about being a constraint (Zeta does not persist attempt
counters/partial-success state across a restart today).
## 3.2 Maintainability
Good. The `XidKey` canonical-comparison type, the `commit`/`restoreCommit`
split, and the retry/backoff helpers are all small, single-purpose, and
documented. The one loose end (li3zhi4 Suggestion 2, `ignoreUnknown=true` with
no production caller) is a minor API-clarity nit, not a maintainability risk.
## 3.3 Extensibility
The reconciliation logic operates purely on canonical XID values and the
existing `XaFacade`/`XaGroupOps` abstractions, so it should extend cleanly to
other XA-capable JDBC dialects without connector-specific branching.
## 3.4 Historical-Version Compatibility
This is the one place where "compatible" needs to be read carefully: this is
a **behavior change**, not a wire/state-format change. No checkpoint state
schema changed (`XidInfo` serialization is untouched), so upgrading a job with
existing checkpointed XA state and then restoring is safe to attempt; the risk
is purely operational — a job that was silently succeeding through a masked
failure before this fix may now fail closed on restore and require manual
investigation, exactly as documented in the incompatible-changes entry. That is
the correct trade-off for a data-correctness fix (a checkpoint that silently
reports success while data is missing is a worse historical-compatibility
problem than a restore that now surfaces the inconsistency), and it is
disclosed rather than silent.
# 4. Issue Summary
No formal (numbered) issues found on this head. All prior concerns from
other reviewers are addressed in the current code; the two remaining items
below are non-blocking carryovers already surfaced by li3zhi4 and not yet
actioned:
| Number | Issue | Location | Severity |
| --- | --- | --- | --- |
| N/A-1 | PR description says restore "fails closed" for an all-absent
batch; code and docs actually treat it as already-resolved | PR description
text only (not code) | Low |
| N/A-2 | `XaFacade.commit(xid, ignoreUnknown=true)` has no production
caller after the evidence-based replay design replaced `XAER_NOTA`-as-success
inference | `JdbcSinkAggregatedCommitter.java`, `XaFacadeImplAutoLoad.java` |
Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge
From a source-correctness standpoint, my independent re-trace of the
ordering invariant, the restore reconciliation logic, and the test evidence
(including the previously-disabled real-MySQL IT test that now genuinely
exercises a live commit failure) finds no blocking issues on the current head
(`ca15d97a9ba`). Both outstanding `CHANGES_REQUESTED` reviews were filed
against earlier heads and target concerns that the current implementation
structurally resolves — nzw921rx's request for recovery-scan-based
reconciliation instead of `XAER_NOTA`-as-success inference, and davidzollo's
non-idempotent-restart scenario. I want to be precise about what I can and
cannot do here: I am not nzw921rx or davidzollo, I cannot dismiss their reviews
on their behalf, and `reviewDecision` will correctly keep showing
`CHANGES_REQUESTED` until one of them re-reviews the current head or a
maintainer with write access revisits and dismisses a stale review. That is a
process blocker, not a code blocker,
and it is the only thing standing between this PR and merge as of this
comment.
Separately, `mergeStateStatus` is `BLOCKED` and the `Build` check is
currently red. I pulled the actual failing job log rather than trusting the
status pointer: `updated-modules-integration-test-part-3` failed after 1 hour
with `Could not resolve dependencies for project connector-jdbc-e2e-part-2: ...
database-commons:1.20.1 ... Connection timed out` — a Maven Central network
timeout unrelated to this PR's code, in a module bucket this PR does not touch
logically. This is the same class of transient infra failure I flagged and
confirmed resolved by rerun in my previous round (that time it was a
`jetty-http` 502 from Maven Central); it needs a rerun, not a code change.
1. Blockers (process, not code): the two outstanding `CHANGES_REQUESTED`
reviews from nzw921rx and davidzollo need to be revisited against the current
head and either re-affirmed with a concrete remaining objection or
dismissed/updated — I cannot do this myself as the PR author. CI needs a clean
rerun given the confirmed-transient failure cause.
2. Recommended non-blocking fixes: correct the PR description's
all-absent-batch wording (li3zhi4 Suggestion 1); consider documenting or
removing the unused `ignoreUnknown=true` production path (li3zhi4 Suggestion 2).
No alternative implementation approach looks better here; the
commit-order-evidence design is the right shape for this problem given the
constraint that Zeta does not 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]
