DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5466087061
Note: this PR is authored by me (DanielLeens), so GitHub blocks a
self-review submission. Posting this as a plain PR comment instead of a formal
review, per project convention for self-authored PRs (same as my previous
rounds on this thread). Since `git diff` between my last comment and the
current head is not empty this time, this is a genuine incremental round, not a
repeat.
New activity since my last comment (`a9257e75bf1a`, then Issue 1 posted
against it): three new commits from @davidzollo — `5ec277d8be97`
("[Docs][Connector-V2] Clarify XA recovery cleanup risk"), `c5f4ec0a0d90`
("[Docs][Connector-V2] Clarify XA recovery concurrency"), and `d2bc272f3641`
("[Chore][Connector-V2] Refresh CI", current head) — plus a new reply from
@davidzollo on @dybyte's Q1 thread. I re-verified each of these directly
against source rather than taking the commit messages or the reply at face
value.
# 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()` 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
I pulled the three new commits as raw diffs from the GitHub API and
cross-checked them against the current head's full source for
`JdbcSinkAggregatedCommitter.java`, `XaGroupOpsImpl.java`,
`XaFacadeImplAutoLoad.java`, and `GroupXaOperationResult.java` rather than
trusting the commit subjects:
- **`5ec277d8be97`** is exactly the one-line migration-guide addition I
recommended as the "Best improvement" for my own Issue 1
(absence-from-recovery-scan cannot distinguish "committed by us" from "rolled
back externally"; only the gap/fail-closed case was previously disclosed). Both
`docs/en` and `docs/zh` `incompatible-changes.md` now state plainly that XA
recovery cannot tell a SeaTunnel-committed XID from one rolled back or removed
by an external actor, and that operators should not run external cleanup
against SeaTunnel-owned prepared branches while a job may still be restored,
without coordinating with recovery. This closes Issue 1 as I scoped it — a
documentation fix, not a code fix, since the underlying gap is
information-theoretic to XA itself, not an implementation defect.
- **`c5f4ec0a0d90`** tightens the `restoreCommit()` Javadoc to name the
external actor explicitly ("a DBA or RM cleanup process"), state plainly that
this is *not* a second Zeta committer, and say outright that the per-batch
recovery-scan refresh "narrows, but cannot eliminate, the gap before the
following XA commit call." I checked this against @davidzollo's reply on
@dybyte's Q1 thread ("Addressed in c5f4ec0a0d") and it is accurate — the new
Javadoc text matches exactly what his inline reply and my own earlier reply in
that same thread already committed to putting in writing. This is a
comment-only change; no executable logic moved.
- **`d2bc272f3641`** (current head) — verified via `gh api
repos/apache/seatunnel/commits/<sha>` that this commit has zero changed files.
It is a genuine empty CI-retrigger commit, not a disguised code change.
I independently re-verified the load-bearing invariants against the current
head's actual source rather than re-trusting my own prior rounds:
- `GroupXaOperationResult.hasNoFailures()` returns `false` if *either*
`failure` (permanent) or `transientFailure` (transient) is present;
`throwIfAnyFailed` only throws on the permanent `failure` field. Combined with
`XaGroupOpsImpl.commit()`'s loop guard `i.hasNext() && (result.hasNoFailures()
|| allowOutOfOrderCommits)` (both production call sites pass
`allowOutOfOrderCommits=false`), the loop stops at the first failure of any
kind and every un-iterated XID is appended to `forRetry` via the same iterator,
preserving order. This is the exact ordering invariant
`JdbcSinkAggregatedCommitter.findFirstRecoveredIndex`/`replayRecoveredCheckpoint`
(lines 207-239) depends on, and it holds in the source I read directly.
- `XaFacadeImplAutoLoad.wrapException` returns (does not throw) a bare
`TransientXaException` for `TRANSIENT_ERR_CODES = {XA_RETRY, XAER_RMFAIL}` and
a `JdbcConnectorException` for everything else, including `XAER_NOTA` and
`XA_RBTRANSIENT`; every one of `execute()`'s two throw sites and both
`Command.fromRunnable*` closures does `throw wrapException(...)`, so
`XaGroupOpsImpl.commit()`'s `catch (XaFacade.TransientXaException e)` is
genuinely reachable. This confirms the core claim of the PR (the pre-fix bug:
the exception used to arrive pre-wrapped inside `JdbcConnectorException`, so
that catch clause was dead code).
Both of these match the six prior rounds' conclusions; I re-derived them
from the current head's source rather than assuming they still hold, since
re-verifying invariants after every commit — even a docs-only one — is the
point of doing another pass at all.
## 1.2 Compatibility Impact
Unchanged: partially incompatible, and now more completely disclosed.
`5ec277d8be97` extends the existing migration-guide breaking-change entry (both
`en` and `zh`) to also cover the prefix/all-absent inference gap, closing the
one disclosure hole this thread had identified. No checkpoint state schema
changed; no source-incompatible API changes in this round.
## 1.3 Performance / Side-Effect Analysis
No change this round — both new substantive commits are
documentation/Javadoc only, and the third is an empty commit. Nothing new to
assess beyond what was already established in prior rounds (bounded
restart-only-path `recover()` scan cost, 1s backoff between retry rounds, O(N)
`containsEquivalentXid` scan per batch).
## 1.4 Error Handling and Logging
No new issues. The two threads open against the current head are both now
answered in the code/docs, not just in comment text:
- @dybyte's Q1 (concurrent-resolution actor) — answered in the thread reply
and now also captured durably in the `restoreCommit()` Javadoc via
`c5f4ec0a0d90`, so the explanation survives independent of the PR discussion.
- My own Issue 1 (prefix/all-absent disclosure gap) — closed via the
migration-guide addition in `5ec277d8be97`.
@dybyte's Q2 (non-blocking ask for a kill-and-restart E2E covering the full
checkpoint -> failure -> restore path) remains an open, explicitly-scoped
follow-up rather than something this round changed — I stand by the reasoning
in my prior reply that a timing-guess-free version of that E2E deserves its own
review rather than being folded in here under time pressure.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Both new substantive commits are comment/doc-only and read clearly; no new
core method or field was added this round, so no new comment-coverage gap to
flag.
## 2.2 Test Coverage and Test Stability: Stable
No test files changed in this round. Unchanged from my prior assessment:
`XaFacadeImplAutoLoadTest`/`XaGroupOpsImplTest` exercise real `XAException`
error codes against mocks, `JdbcSinkAggregatedCommitterTest` covers every
reconciliation branch (prefix-skip, all-absent skip, fail-closed-on-gap,
transient retry, retry-exhaustion), and `XaGroupOpsImplIT` now runs a real
MySQL Testcontainer end to end instead of being `@Disabled`.
## 2.3 Documentation Updates
Improved this round, not just unchanged: the migration guide gap I flagged
as Issue 1 is now closed in both `docs/en` and `docs/zh`.
`docs/en/connectors/sink/Jdbc.md`/`docs/zh` remain accurate against the code as
previously verified.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution: Precise fix
Unchanged.
## 3.2 Maintainability
Unchanged.
## 3.3 Extensibility
Unchanged.
## 3.4 Historical-Version Compatibility
Unchanged: no wire/state-format change; the operational risk is disclosed,
and this round makes that disclosure more complete rather than introducing
anything new to assess.
# 4. Issue Summary
| Number | Issue | Location | Severity |
| --- | --- | --- | --- |
| 1 (closed) | Absent-from-recovery-scan XIDs could not be distinguished
from an external rollback, and only the gap/fail-closed case was disclosed |
`docs/en(zh)/introduction/concepts/incompatible-changes.md` | Was Medium —
resolved by `5ec277d8be97` |
| N/A-2 | `XaFacade.commit(xid, ignoreUnknown=true)` still 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 still 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 |
No new blocking issues found on this head.
# 5. Merge Recommendation
### Conclusion: Ready to merge
From a source-correctness standpoint, this round's two substantive commits
do exactly what their messages and the accompanying thread replies claim — I
verified both against the actual diffs and current source rather than trusting
the commit subjects, and the third commit is a genuinely empty CI retrigger. My
own Issue 1 is now closed by documentation rather than left as a
recommendation. No new code-level issue surfaced.
Restating the process status precisely, since it's easy to overstate:
`reviewDecision` is currently `CHANGES_REQUESTED`, driven solely by @nzw921rx's
2026-07-27 review — which, as established in my 2026-08-29 full re-review,
predates the recovery-scan reconciliation mechanism entirely and targets a
design that no longer exists in this form. @davidzollo's 2026-08-06
`CHANGES_REQUESTED` no longer counts toward `reviewDecision` because his latest
review on this thread is `COMMENTED` (his concrete non-idempotency scenario was
fixed in code back in `97b0942d7b2a`, and he has continued actively pushing
fixes and replying since) — but I want to be precise that GitHub superseding
his review state for computation purposes is not the same as him having
formally said "resolved, no objection," so I'm not claiming more than the
mechanical fact here. @dybyte's `APPROVED` review was auto-dismissed by branch
protection when the `5ec277d8be97` commit landed, which is expected
push-triggered be
havior, not a new objection — nothing in that thread asked for anything this
round didn't already close out.
CI: the `Build` check on the current head (`d2bc272f3641`) is still in
progress on the fork as I write this (run `33278167729`, ~3.5h in). Three jobs
have already failed early — `updated-modules-integration-test-part-2`,
`updated-modules-integration-test-part-3`, and `doris-connector-it` (all on JDK
8, ubuntu-latest) — but their logs aren't retrievable yet while the overall run
is still active, so I can't pull the exact failure lines this time the way I
did in my previous four CI-triage rounds on this same PR. What I can say
factually: the first two of those three job buckets have failed and been
diagnosed as unrelated Maven Central network flakes or an unrelated
`PostgresCDCIT` timing flake in every prior round on this exact PR (this PR's
diff has never touched `connector-jdbc-e2e-part-2`, `connector-milvus`,
`connector-cdc-postgres`, or `connector-doris`), so this is consistent with —
but not yet independently re-confirmed as — the same recurring infra-flake
pattern. I'm
flagging that distinction rather than asserting the current failures are
flakes before I've actually read their logs.
1. Blockers (process, not code): @nzw921rx's stale `CHANGES_REQUESTED` needs
to be revisited against the current head and either re-affirmed with a concrete
remaining objection or dismissed/updated by them or a maintainer with write
access — I cannot do this myself as the PR author. CI is still running on the
current head with three early failures whose logs aren't yet available; once it
completes, those logs should be pulled to confirm they match the established
unrelated-infra pattern before assuming a rerun is all that's needed.
2. Recommended fixes (non-blocking): the two low-severity carryovers from
li3zhi4 (PR description wording, unused `ignoreUnknown=true` path) remain open
and are cheap to fold in whenever this PR is touched again.
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]
