DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5492604274
Since this is my own PR, GitHub won't let me submit a formal review state on
it, so this is posted as a plain comment. I re-reviewed the current head from
scratch, treating the whole diff and runtime path as if I hadn't written it,
rather than skimming the delta.
# What Problem Does This PR Solve?
- User pain point: before this PR, a permanent JDBC XA commit failure could
be silently swallowed instead of propagated —
`GroupXaOperationResult.throwIfAnyFailed("commit")` was commented out, and
`XaFacadeImplAutoLoad.wrapException()` always pre-wrapped
`TransientXaException` inside a `JdbcConnectorException`, so
`XaGroupOpsImpl.commit()`'s `catch (XaFacade.TransientXaException e)` branch
was dead code. A prepared XA transaction could fail to commit and vanish
without being retried, reported, or rolled back, while the checkpoint was still
reported complete — a real data-loss risk for a two-phase-commit sink.
- Fix approach: restore failure propagation, correct transient-vs-permanent
XA error classification, add bounded synchronous retry with backoff, and add a
`restoreCommit()` reconciliation path that consults a live `xaFacade.recover()`
scan in commit order before replaying restored, still-in-doubt transactions.
- One-sentence summary: this re-review confirms the core JDBC XA fix is
unchanged and correct; the only new commit since my last review is an unrelated
E2E test-readiness hardening for two other connectors (Azure CosmosDB, Milvus)
that happens to concretely fix the two CI failures I flagged as unrelated in my
last round.
I re-fetched the head myself (`git log -1` on a fresh clone of
`DanielLeens/seatunnel` at `davidzollo_fix_jdbc_xa_commit_failure_20260727`)
and confirmed it moved from `18bfbd1743bd` (my last review) to `fbf5b7c99c0d`,
one commit: `[Fix][E2E] Wait for Cosmos and Milvus service readiness`. I diffed
`18bfbd17..fbf5b7c9` directly and confirmed it touches only
`AbstractAzureCosmosDBIT.java` and `MilvusIT.java` — not `XaFacade.java`,
`XaFacadeImplAutoLoad.java`, `XaGroupOps.java`, `XaGroupOpsImpl.java`,
`GroupXaOperationResult.java`, or `JdbcSinkAggregatedCommitter.java`. I did not
stop at the diff stat, though — for a proper re-review I re-read every one of
those core files end to end at the current head and independently re-traced the
full runtime chain myself (not by re-reading my own prior write-up), documented
below.
No new review/comment activity from anyone else landed after my Sept 1
00:30/03:54 rounds at `18bfbd17`, so there's nothing new to acknowledge from
other reviewers this round. I did not find anything in this pass that should
have been caught in an earlier round but wasn't — my own carried-forward Issue
4 (below) already covers the one non-trivial finding this deep re-trace
produced, so no "sorry, missed this" correction is warranted here.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Runtime chain, independently re-traced against the current checked-out head:
```text
notifyCheckpointComplete path (live commit):
CheckpointCoordinator.notifyCompleted()
-> notifyCheckpointCompleted() -> CheckpointFinishedOperation.runInternal()
-> task.notifyCheckpointComplete(checkpointId)
SinkAggregatedCommitterTask.java:304-322
-> aggregatedCommitter.commit(aggregatedCommitInfo)
-> JdbcSinkAggregatedCommitter.commit() ->
commitPreparedTransactions() -> commitXidInfos()
-> xaGroupOps.commit(pending, false, maxCommitAttempts)
XaGroupOpsImpl.java:50-83
-> xaFacade.commit(xid, false) per XID, in order
-> permanent failure: result.failed(x, e)
-> transient failure (XA_RETRY / XAER_RMFAIL only):
result.failedTransiently(...)
-> result.throwIfAnyFailed("commit") throws on any
permanent failure
-> exception propagates out of notifyCheckpointComplete (declared
`throws Exception`, no catch)
-> CompletableFuture.allOf(...).join() in notifyCompleted() throws
-> handleCoordinatorError(..., CHECKPOINT_NOTIFY_COMPLETE_FAILED)
CheckpointCoordinator.java:341-357
-> updateStatus(FAILED);
checkpointManager.handleCheckpointError(pipelineId, false)
restoreState path (job restart):
SinkAggregatedCommitterTask.restoreState()
SinkAggregatedCommitterTask.java:260-282
-> aggregatedCommitter.restoreCommit(aggregatedCommitInfos)
-> per batch: replayRecoveredCheckpoint(checkpointXids,
recoverCheckpointTransactions())
-> findFirstRecoveredIndex(): first checkpoint XID still present in
a fresh xaFacade.recover() scan
-> everything before that index: treated as already resolved (skip,
log WARN)
-> everything from that index onward: must all be present in the
scan (fail closed if not) -> commitXidInfos()
```
I did not just re-read my own prior write-up of this chain — I re-derived
the checkpoint-failure propagation independently this round by reading
`CheckpointFinishedOperation.runInternal()` and
`CheckpointCoordinator.notifyCompleted()`/`handleCoordinatorError()` myself,
since that's the part of the chain outside the JDBC connector module that
actually proves a thrown `JdbcConnectorException` really fails the checkpoint
rather than being swallowed somewhere in the engine's operation-retry wrapper.
Confirmed: `RetryUtils.retryWithException` in `CheckpointFinishedOperation`
only retries `TaskGroupContextNotFoundException`, so our exception is wrapped
once in `SeaTunnelEngineException` and rethrown, and `notifyCompleted()`'s
`catch (Throwable e)` routes it to `handleCoordinatorError`, which marks the
checkpoint coordinator `FAILED` and triggers
`checkpointManager.handleCheckpointError`. This is the fix actually working
end-to-end, not just failing loudly inside the connector and gett
ing lost above it.
**One finding worth stating precisely (carried forward as Issue 1 below, not
new):** `XaFacade.commit(xid, ignoreUnknown)` is hardcoded to `false` at its
only production call site (`XaGroupOpsImpl.java:61`), so `XAER_NOTA` ("Xid is
not valid to RM") is never in `TRANSIENT_ERR_CODES` and always surfaces as a
permanent failure. I traced the concrete scenario this matters for: if an
external actor (a DBA, or RM in-doubt-transaction cleanup tooling) resolves a
checkpoint's XID between `recover()` and the following `xaFacade.commit()` call
for the same batch — the residual gap `replayRecoveredCheckpoint()`'s Javadoc
explicitly documents as narrowed-but-not-eliminated — the live commit or
restore call fails hard instead of silently treating the already-resolved XID
as done. I traced what happens next: the resulting job restart re-enters
`restoreState()`/`restoreCommit()`, which re-scans and this time correctly
finds that XID absent-and-skippable (since it's now genuinely resolved),
so the batch converges within one extra restart cycle rather than looping
forever. This is a self-healing, operator-visible "spurious restart," not a
livelock and not silent data loss — and it is the intentional, safer trade-off
versus the alternative (treating any `XAER_NOTA` as "fine, already resolved,"
which would also mask a genuinely corrupted/fabricated Xid). This exact race
and trade-off is already discussed at length by `@dybyte` and me in the
`JdbcSinkAggregatedCommitter.java:105` review thread (2026-08-27), and the
`restoreCommit()` Javadoc was tightened in `c5f4ec0a0d` to name the
external-actor scenario explicitly — so this is confirmation of
already-surfaced, already-addressed-in-docs behavior, not a new gap.
## 1.2 Compatibility Impact
**Partially incompatible, and disclosed** — unchanged from prior rounds. No
wire/state serialization format change. The observable behavior change (a
permanent XA commit failure now fails the checkpoint instead of being silently
absorbed, and a missing XID after a still-prepared one fails restore closed
instead of assuming success) is the intended fix and is documented in
`docs/en/introduction/concepts/incompatible-changes.md` / `docs/zh/...` under
"JDBC XA restore now uses recovery-order evidence and fail-closed gaps." I
re-read the current doc text against the current code myself this round rather
than trusting the prior summary, and it accurately matches
`replayRecoveredCheckpoint()`'s actual behavior (prefix-skip,
suffix-fail-closed, all-absent-skip).
## 1.3 Performance / Side-Effect Analysis
Unchanged from prior rounds: bounded retry (`maxCommitAttempts`, default 3,
1s fixed backoff via `Thread.sleep` in `backoffBeforeRetry`), executed on the
aggregated committer's own dedicated task thread (not a shared engine thread
pool), triggered only at checkpoint-complete/restore boundaries, not
per-record. No unbounded collections — `pending`/`toRetry`/`stillPrepared` are
all bounded by the batch's own XID count.
## 1.4 Error Handling and Logging
No swallowed exceptions on the paths I traced. Every hard-failure path
throws a `JdbcConnectorException` naming the affected XIDs (or wraps a
`TransientXaException`/`XAException` with its error code), and log levels are
appropriate (`WARN` for skip/already-resolved/retry-exhaustion-adjacent
conditions, `ERROR`-equivalent via the thrown exception for real failures). No
credentials or connection strings appear in any of the touched log statements.
## 2. Code Quality Assessment
## 2.1 Coding Standards
Javadoc present on all new/changed non-trivial methods (`restoreCommit`,
`replayRecoveredCheckpoint`, `recoverCheckpointTransactions`,
`findFirstRecoveredIndex`, `containsEquivalentXid`, `backoffBeforeRetry`), ASF
license headers present, no wildcard imports in any touched file.
## 2.2 Test Coverage and Test Stability
**Core JDBC/XA logic: Stable.** Unit coverage (`XaFacadeImplAutoLoadTest`,
`XaGroupOpsImplTest`, `JdbcSinkAggregatedCommitterTest`) exercises every
reconciliation branch (recovered-by-value matching, unrelated recovered XIDs,
already-resolved prefixes, no-evidence missing gaps failing closed, bounded
transient-retry for both commit and the recovery scan). `XaGroupOpsImplIT` runs
against a real MySQL testcontainer, not a mock, and its
`testCommitFailurePropagatesThroughAggregatedCommitter` drives an actual XA
prepare -> forced commit failure -> propagation through the real committer.
I independently verified the current CI run for this exact head
(`fbf5b7c99c0d`, fork run `33476561911`) rather than trusting the prior round's
numbers: 78 success, 11 skipped, 2 still in progress at review time, 2 failures
— `edge-agent-it (8)` and `doris-connector-it (8)`, both clearly unrelated to
this diff's connector/module. Every `jdbc-connectors-it-part-*` (1-7, both
JDKs), `jdbc-connectors-it-ddl` (both JDKs), and `unit-test` (all four OS/JDK
combinations) job is green. Notably, `all-connectors-it-4` and
`all-connectors-it-7` — the two jobs I diagnosed as pre-existing/unrelated
flakes in my last round (`AzureCosmosDBSourceIT` and `MilvusIT` failures) — are
now green on both JDKs in this run, which is concrete evidence that this
round's new commit (the Cosmos/Milvus readiness-wait hardening) actually fixed
those two flakes rather than just being incidental scope creep.
**Stability rating for the new commit's own test-infra change: Stable.**
`seedContainerWhenReady`/`waitForMilvusReady` use
`Awaitility.await().ignoreExceptions().pollInterval(...).atMost(...).untilAsserted(...)`
— a real readiness assertion (retrying the actual seed/RPC call until it
succeeds), not a bare fixed sleep. `MilvusIT` previously used a bare
`Awaitility.given().ignoreExceptions().await().atMost(720L, SECONDS)` with no
assertion inside `await()` at all (that call by itself waits the fixed duration
unconditionally since nothing is being asserted) — replacing it with a real
`listDatabases()` RPC-backed readiness check is a strict improvement, not a
loosened tolerance: it now fails fast if the real precondition isn't met
instead of just sleeping a fixed window and hoping.
## 2.3 Documentation Updates
Unchanged from prior rounds — `docs/en(zh)/connectors/sink/Jdbc.md` and
`docs/en(zh)/introduction/concepts/incompatible-changes.md` are consistent with
the current code, verified again this round (see 1.2).
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Precise fix: restores previously-disabled failure propagation, corrects a
specific error-code misclassification (`XA_RBTRANSIENT` — already permanently
rolled back — replaced with `XA_RETRY` — call had no effect, safe to reissue —
in `TRANSIENT_ERR_CODES`), and adds order-preserving, evidence-based restore
reconciliation rather than a new persisted-state mechanism.
## 3.2 Maintainability
Good — the reconciliation invariant (`XaGroupOpsImpl.commit()` only ever
attempts XIDs in original order with `allowOutOfOrderCommits=false`, so a
still-prepared XID at index *k* implies everything after *k* in the same batch
must also still be prepared) is now explicit in both code comments and the
incompatible-changes doc, not left implicit.
## 3.3 Extensibility
No concerns raised this round.
## 3.4 Historical-Version Compatibility
No checkpoint/savepoint state-schema change — `XidInfo`'s serialized shape
is untouched, only the in-memory reconciliation logic around it changed. A job
with in-flight XA-prepared transactions from before this upgrade restores
exactly the same way as one prepared after it.
# 4. Issue Summary
| Number | Issue | Location | Severity | Raised by another reviewer |
| --- | --- | --- | --- | --- |
| 1 (carried) | `XaFacade.commit(xid, ignoreUnknown=true)` still has no
production caller; `ignoreUnknown` is hardcoded `false` at the only call site,
so an externally-resolved XID (DBA/RM cleanup) always surfaces as a hard
failure rather than being tolerated. Self-healing within one restart cycle
(traced in 1.1), and the trade-off is intentional, but the unused `true` branch
is dead code today. | `JdbcSinkAggregatedCommitter.java` (call site) /
`XaFacadeImplAutoLoad.java` (`buildCommitErrorDesc`) | Low | Yes — discussed at
length with @dybyte (2026-08-27); Javadoc hardened in `c5f4ec0a0d` |
| 2 (carried) | PR description text previously described the all-absent case
as "fails closed," while code/docs correctly treat it as "already resolved" |
PR description | Low | No (self-carried) |
No High or Medium issues found this round or carried from before.
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed (process, not code; I cannot act on these
myself as the PR author):**
- `reviewDecision` is still `CHANGES_REQUESTED`, driven by two
still-open, unresolved formal reviews: `@nzw921rx` (2026-07-27, at commit
`f836f8d6`) and `@davidzollo` (2026-08-06, at commit `8bb49c06`). Both predate
the current recovery-scan/commit-order reconciliation design by many rounds.
Neither reviewer has formally dismissed or updated their review state on GitHub
as of this comment. This needs a maintainer with write access to re-review and
clear the stale gate, or for `@nzw921rx`/`@davidzollo` to update their own
review state — I cannot dismiss another reviewer's formal review, and being the
PR author I cannot submit my own formal review state at all (hence this plain
comment).
- CI on the current head still has 2 in-progress jobs at review time;
confirm the run finishes with no new failures beyond the two
already-diagnosed-unrelated ones (`edge-agent-it`, `doris-connector-it`).
2. **Recommended fixes — non-blocking:**
- Issue 1 (Low): remove the dead `ignoreUnknown=true` capability, or
leave it as documented, deliberate defense against `XAER_NOTA` being conflated
with "this transaction was legitimately externally resolved" — either is
defensible, but the current unused-branch state should be a conscious choice,
not an oversight.
- Issue 2 (Low): tighten the PR description's own wording to match the
code/docs ("treated as already resolved," not "fails closed") for the
all-absent-batch case.
**Overall assessment:** this re-review found the core fix unchanged,
correct, and — where I dug deeper than a diff-stat check (the
checkpoint-failure propagation chain outside the connector module, and the
`XAER_NOTA`/external-actor race) — independently re-derivable from the source
rather than merely plausible. The one new commit this round is a well-targeted,
unrelated E2E test-readiness hardening that concretely fixed the two flaky CI
jobs I'd previously diagnosed as unrelated noise. The two open blockers remain
purely procedural: stale `CHANGES_REQUESTED` states from reviews that predate
the current design, and CI finishing. A different maintainer needs to action
the review-gate item; I can't do that on my own PR.
--
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]
