DanielLeens commented on PR #11569:
URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5461958108
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).
New activity since my last full re-review (comment above, at head
`ca15d97a9bad`): one new commit, `a9257e75bf1a` ("[Chore] Retry CI"), and a new
`APPROVED` review from @dybyte on the prior head. I re-ran the analysis below
from scratch against the current head rather than assuming the prior round
still applies.
# What Problem Does This PR Solve?
Unchanged from my last full re-review: before this patch, a permanent JDBC
XA commit failure could be silently swallowed (`XaGroupOpsImpl.commit()`
recorded the failure but `throwIfAnyFailed("commit")` was disabled, and
`wrapException()` always wrapped `TransientXaException` so the retryable-error
branch was unreachable). This PR restores failure propagation, fixes
retryable/permanent error classification, bounds retries within a single
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 diffed the current head against the exact commit I fully re-reviewed less
than a day ago:
```
git diff ca15d97a9bad a9257e75bf1a9fb63bcdaca3130baa3c9c585990
```
This is empty. `a9257e75bf1a` is a genuine zero-diff commit (verified via
`git show --stat`, no files listed) authored by @davidzollo purely to retrigger
CI — not a rebase, not a squash-with-hidden-change, an actual empty commit on
top of the same tree. So the full from-scratch analysis I already posted for
`ca15d97a9bad` (ordering invariant in `XaGroupOpsImpl.commit()`, the
`findFirstRecoveredIndex`/`replayRecoveredCheckpoint` reconciliation, the
nzw921rx and davidzollo scenarios, li3zhi4's three suggestions) stands
unchanged on the current head byte-for-byte — I re-verified this via the diff
above rather than assuming it. I'm not going to re-paste that ~2000-word
analysis verbatim here since it would add no new information; it's the comment
immediately above this one on the same head-equivalent code.
What actually changed this round is CI, addressed in full below.
## 1.2 Compatibility Impact
Unchanged: partially incompatible, correctly disclosed in
`docs/en(zh)/introduction/concepts/incompatible-changes.md` — jobs relying on
the old silent-success-on-gap restore behavior will now fail closed and need
operator investigation.
## 1.3 Performance / Side-Effect Analysis
Unchanged: bounded, restart-only-path `recover()` scan cost; 1s backoff
between synchronous retry rounds only fires under genuine XA/RM instability.
## 1.4 Error Handling and Logging
No new issues. All previously raised concerns (nzw921rx, davidzollo) remain
structurally resolved on this head, since the code is identical to what I
already verified line-by-line.
**CI diagnosis for the current head (`a9257e75bf1a`)** — this is the new,
real work this round. `Build` is red on the apache-side status, which is just a
pointer; the real run is on the fork. I pulled every failing job's actual log
rather than trusting the red X:
- `Run / unit-test (8, ubuntu-latest)` —
`CoordinatorServiceTest.testClearCoordinatorServiceDropsPendingJobsUnderRejectStrategy:642
» ConditionTimeout` (Awaitility timeout waiting on job-scheduling state in
`seatunnel-engine-server`). This PR's diff (verified via `git diff <merge-base
43fe63b1fca> a9257e75bf1a` — the correct comparison, since `dev` has moved ~25
commits past this PR's merge-base and diffing against current `dev` tip pulls
in unrelated noise) touches only `connector-jdbc`'s XA classes, its own tests,
`connector-jdbc-e2e-part-1`, and docs. It does not touch
`seatunnel-engine-server`, `CoordinatorService`, or anything in its call path.
I also checked: no commit between this PR's merge-base and current `dev`
touched `CoordinatorServiceTest.java` or `CoordinatorService.java`, so this
isn't a case of a known-fixed flake regressing — it's an Awaitility-based
engine unit test racing against Hazelcast job-scheduling state, in a module
this PR never touches.
- `Run / updated-modules-integration-test-part-3 (8, ubuntu-latest)` —
`connector-jdbc-e2e-part-2` (not this PR's own `connector-jdbc-e2e-part-1`)
failed at the Maven dependency-resolution stage: `Could not transfer artifact
com.google.code.gson:gson:pom:2.13.1 ... Connection reset`, a transitive
dependency of `connector-milvus`. Pure Maven Central network flake, same class
of failure (different artifact) as the `database-commons` timeout I diagnosed
on the previous head.
- `Run / updated-modules-integration-test-part-2 (8, ubuntu-latest)` and
`(11, ubuntu-latest)` — both failed on the exact same test,
`PostgresCDCIT.testPostgresCdcSnapshotOnlyAndCommittedOffsetStartupModes:614 »
ConditionTimeout` (`expected: <1> but was: <0> within 3 minutes`), on two
different JDKs in the same run. `PostgresCDCIT` belongs to
`connector-cdc-postgres`, which this PR does not touch at all. The identical
failure signature across two independent JDK runs is itself evidence this is a
pre-existing environmental/timing flake in the CDC container setup, not
something a code change in this run introduced (there is no code change in this
run).
None of the four failures touch a file, module, or call path this PR's diff
modifies. This needs a CI rerun, not a code change — consistent with the
transient-infra pattern from my previous two CI triage rounds on this same PR.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged: all new/changed core methods carry Javadoc; no gaps found on the
previous from-scratch pass, and there's no code delta to re-check.
## 2.2 Test Coverage and Test Stability: Stable
Unchanged rating. No test files changed in this commit. The genuinely new
signal this round is negative-but-irrelevant: none of the newly-failing CI
tests belong to this PR's own test suite (`XaFacadeImplAutoLoadTest`,
`XaGroupOpsImplTest`, `JdbcSinkAggregatedCommitterTest`, `XaGroupOpsImplIT`) —
those all still pass in every failing job's log; only unrelated modules failed.
## 2.3 Documentation Updates
Unchanged: `docs/en(zh)/connectors/sink/Jdbc.md` and the
incompatible-changes docs are current and match the code; no doc delta in this
commit.
# 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 checkpoint state schema changed; the risk is purely
operational and disclosed.
# 4. Issue Summary
No new code-level issues on this head (no code changed). Carrying forward
the two non-blocking items from my previous round, unrenumbered:
| 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
The code on the current head is byte-for-byte identical to what I already
fully re-traced (ordering invariant, restore reconciliation, both outstanding
`CHANGES_REQUESTED` scenarios, li3zhi4's suggestions) in my previous comment —
I verified that identity directly via diff rather than assuming it, so this is
a genuine re-review conclusion, not a copy-paste. @dybyte's `APPROVED` review
on the prior head is further independent confirmation.
What's actually new this round is the CI failure, and I traced it to four
jobs, none of which touch a file this PR's diff modifies: an unrelated engine
unit test (`CoordinatorServiceTest`), a Maven Central network reset resolving
an unrelated module's dependency (`connector-jdbc-e2e-part-2`/`gson`), and the
same pre-existing `PostgresCDCIT` Awaitility timeout on two JDKs. This is infra
flake, not a regression from this commit (which changed no files) or from this
PR's diff (which never touches any of the three affected modules).
1. Blockers (process, not code, same as last round): the two outstanding
`CHANGES_REQUESTED` reviews from @nzw921rx and @davidzollo need to be revisited
against the current head and re-affirmed or dismissed by them or a maintainer
with write access — I cannot do this myself as the PR author. CI needs a clean
rerun given the four confirmed-unrelated failure causes above.
2. Recommended non-blocking fixes: same two carryovers as before (PR
description wording, unused `ignoreUnknown=true` path).
No alternative implementation approach changes here; nothing in this round's
findings affects the design.
--
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]
