DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5488644802
# What Problem Does This PR Solve?
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. The fix restores failure
propagation, corrects
transient-vs-permanent XA error classification, adds bounded synchronous
retry with backoff, and
adds a `restoreCommit()` reconciliation path that consults a live
`xaFacade.recover()` scan in
commit order before replaying restored, still-in-doubt transactions.
# Re-review context (why this round happened)
This is a follow-up check, not a new code round. The head SHA is unchanged
since my last full
review: `18bfbd1743bde7b9ab270545665139f5ad0f72f6`. I confirmed this
directly (`git log -1` on a
fresh fetch of `refs/pull/11569/head`, and `gh pr view --json headRefOid`),
and re-read
`JdbcSinkAggregatedCommitter.java` at that exact commit to double check
nothing drifted between my
last review and this one — it hasn't. My prior review already did the full
source-level trace of
`XaFacadeImplAutoLoad`, `XaGroupOpsImpl`, `GroupXaOperationResult`, and
`JdbcSinkAggregatedCommitter.restoreCommit()`/`replayRecoveredCheckpoint()`,
and I stand by that
analysis unchanged (see 1.1 below for a condensed restatement rather than a
full re-derivation).
What's actually new in this round is CI progress: at my last review the
fork's `Build` run for this
exact head had just been queued (0 jobs finished). It has now run
substantially further.
# 1. Code Change Review
## 1.1 Core Logic Analysis (condensed restatement, unchanged from my last
pass)
- `XaFacadeImplAutoLoad.TRANSIENT_ERR_CODES` correctly changed from
`{XA_RBTRANSIENT, XAER_RMFAIL}`
to `{XA_RETRY, XAER_RMFAIL}`. `XA_RBTRANSIENT` means the branch is already
rolled back (a done,
permanent outcome); `XA_RETRY` means the call had no effect and may be
safely reissued. The old
code retried an already-rolled-back branch, which could never succeed.
- `wrapException()` now returns `TransientXaException` bare instead of
nesting it inside
`JdbcConnectorException`, which is what makes `XaGroupOpsImpl.commit()`'s
`catch (XaFacade.TransientXaException e)` reachable for the first time.
All three call sites
still `throw wrapException(...)` — verified.
- `restoreCommit()`/`replayRecoveredCheckpoint()` reconciles checkpoint XIDs
against a fresh
recovery scan using commit order: everything before the first
still-prepared XID is treated as
already resolved, the still-prepared suffix is replayed strictly, and a
gap after the first
still-prepared XID fails closed (this relies on `XaGroupOpsImpl.commit()`
only ever attempting
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
— a genuine mismatch
there is real evidence of inconsistency, not something to guess past).
**Runtime path reached: yes, this is the normal restore path**, not a rare
corner case —
`restoreCommit()` runs on every task restart with in-flight XA checkpoint
state.
## 1.2 Compatibility Impact
Unchanged from my last review: partially incompatible, and disclosed. No
wire/state serialization
format change. The observable behavior change — a permanent XA commit
failure now fails the
checkpoint instead of being silently absorbed — is the intended fix and is
documented in
`docs/en(zh)/introduction/concepts/incompatible-changes.md`.
## 1.3 Performance / Side-Effect Analysis
Unchanged: bounded retry (`maxCommitAttempts` default 3, 1s backoff), runs
on the aggregated
committer's own dedicated task thread, not a shared engine-wide thread. No
unbounded structures.
## 1.4 Error Handling and Logging
No new issues found this round. Same as before: every hard-failure path
throws a
`JdbcConnectorException` naming the affected XIDs, and logging levels are
appropriate.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged from my last review — Javadoc present on all new/changed
non-trivial methods, ASF
headers present, no wildcard imports.
## 2.2 Test Coverage and Test Stability
**Core JDBC/XA logic: Stable**, unchanged assessment. What's new this round
is that the CI jobs
that actually exercise this coverage have now run:
- `jdbc-connectors-it-part-1` through `jdbc-connectors-it-part-7`, both JDK
8 and JDK 11 (14 jobs
total): **all green.** `jdbc-connectors-it-part-1` carries the re-enabled
`XaGroupOpsImplIT`
(previously `@Disabled` for a MySQL-driver classloading reason), which
this PR's own real-MySQL
test of failure propagation through the aggregated committer depends on.
This directly closes the
Low-severity "verify via CI" caveat I flagged as Issue 1 in my prior
review — the re-enable is
now proven, not just plausible.
- `unit-test`, both JDK 8 and JDK 11: **all green.** This carries
`XaFacadeImplAutoLoadTest`/`XaGroupOpsImplTest`/`JdbcSinkAggregatedCommitterTest`
and the
`CoordinatorServiceTest` change I flagged as Issue 2 (four consecutive
commits stabilizing the
same test). Green on this run closes that verification request too.
**Stability rating: Stable** (unchanged from last review — no
`Thread.sleep`, no shared static
state, no order-dependence, no floating-point comparisons in any of the
touched test code).
## 2.3 Documentation Updates
Unchanged — already covered by earlier rounds and not affected by this round.
# 3. Architectural Soundness
Unchanged from my last review — precise fix, good maintainability and
extensibility, no checkpoint
state schema change (see prior review for full detail; nothing in this
round's diff touches these
dimensions since there is no new diff).
# 4. Issue Summary
| # | Issue | Location | Severity | Status |
| --- | --- | --- | --- | --- |
| 1 | `XaGroupOpsImplIT` re-enable wasn't proven by the diff alone |
`.../connector-jdbc-e2e-part-1/.../XaGroupOpsImplIT.java` | Low | **Resolved
this round** — `jdbc-connectors-it-part-1` is green on both JDK 8 and 11 in
fork run `33453354584` |
| 2 | `CoordinatorServiceTest` re-touched four rounds in a row for the same
race | `.../engine/server/CoordinatorServiceTest.java` | Low | **Resolved this
round** — `unit-test` is green on both JDK 8 and 11 in the same run |
| 3 | Two unrelated CI job failures in the broader `all-connectors-it`
matrix | see CI diagnosis below | Informational, not a PR blocker | New this
round, diagnosed below |
| 4 (carried) | `XaFacade.commit(xid, ignoreUnknown=true)` still has no
production caller | `JdbcSinkAggregatedCommitter.java` /
`XaFacadeImplAutoLoad.java` | Low | Open, unchanged |
| 5 (carried) | PR description still describes the all-absent case as "fails
closed" when code/docs treat it as already-resolved | PR description text | Low
| Open, unchanged |
No High or Medium issues found this round or carried from before. Issues 1
and 2 from my prior
review are now closed by the CI evidence below; Issue 3 is a fresh CI
observation, diagnosed as
unrelated to this PR's diff.
## CI diagnosis (task-relevant detail)
Fork `Build` run for the current head:
`https://github.com/DanielLeens/seatunnel/actions/runs/33453354584`
(the apache-side "Build" check is a pointer to this run, per this repo's
usual CI wiring).
- 86 of 93 jobs completed, 7 still `in_progress` at the time of this comment
(`engine-v2-it`, `mysql-cdc-connector-it` x2 JDK, `connector-redis-it` x2
JDK,
`kafka-connector-it` x2 JDK — none of these touch the JDBC XA code path).
- 84 of the 86 completed jobs are green, including every
`jdbc-connectors-it-part-*` (1-7, both
JDKs) and every `unit-test` (both JDKs) — the jobs that actually exercise
this PR's own diff.
- 2 failures, both clearly unrelated to this PR:
- `all-connectors-it-4 (8, ubuntu-latest)`:
`org.apache.seatunnel.e2e.connector.azurecosmosdb.AzureCosmosDBSourceIT`
— an Azure Cosmos DB
source test, a connector this PR does not touch.
- `all-connectors-it-7 (11, ubuntu-latest)`:
`org.apache.seatunnel.e2e.connector.v2.milvus.MilvusIT` — a Milvus test,
also a connector this
PR does not touch.
- The rest of the
`all-connectors-it-*`/`updated-modules-integration-test-part-*` split shows the
usual round-robin-bucket pattern for this repo's CI (some
`updated-modules-*` legs `skipped`
because the broader `all-connectors-it-*` legs ran instead) — this is
expected bucket routing,
not a coverage gap.
- Conclusion: nothing in the current CI signal implicates this PR's JDBC/XA
change. The two
failures are pre-existing/unrelated-connector flakes in a shared matrix
job, and every job that
actually covers this diff is green.
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
This round found no new code-correctness issues — there is no new code to
find them in, the diff is
byte-identical to what I already fully reviewed. The two Low-severity
CI-verification caveats from
my prior review are now closed with real, observed green results, not just
plausibility arguments.
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) and `@davidzollo` (2026-08-06). Both
predate the current
recovery-scan/commit-order reconciliation design and, based on my own
independent source-level
re-derivation in earlier rounds of this review, are addressed by the
code as it stands today.
Neither reviewer has formally dismissed or updated their review state
on GitHub. This needs a
maintainer with write access to re-review and clear the stale gate, or
`@nzw921rx` /
`@davidzollo` to do so themselves — I cannot dismiss another reviewer's
formal review.
- Let the remaining 7 in-progress CI jobs finish. None of them touch this
PR's own code path, but
a fully green run (rather than a run with jobs still pending) should be
confirmed before merge.
2. **Recommended fixes — non-blocking, unchanged from before:**
- Carried Issue 4 (Low): remove or wire up the unused
`ignoreUnknown=true` production path.
- Carried Issue 5 (Low): tighten the PR description's wording on the
all-absent-batch behavior to
match the code/docs ("treated as already resolved," not "fails closed").
**Overall assessment:** the core fix is sound and has converged, not
regressed, across every round
of review on this PR — I have no new source-level concerns this round
because there is no new
source in this round. The two open blockers are both procedural: a stale,
unresolved
`CHANGES_REQUESTED` state from reviews that predate the current design, and
a CI run that hasn't
finished yet (but is green everywhere relevant to this diff so far). Once a
maintainer clears the
stale review gate and the remaining CI jobs finish green, I see nothing left
blocking merge.
Since this is my own PR, this is posted as a comment rather than an
approval/changes-request —
GitHub blocks self-review states. A different maintainer needs to action the
review-gate item above.
--
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]
