DanielLeens commented on PR #11569: URL: https://github.com/apache/seatunnel/pull/11569#issuecomment-5473674263
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 review, per project convention for self-authored PRs (same as my previous rounds on this thread). New activity since my last comment (`d2bc272f3641`, 2026-08-30T01:50:21Z): four real commits plus one empty CI-retrigger commit, current head `5eefa194acda`: - `7a951674dafb` "[Fix][E2E] Stabilize CDC and Doris error tests" - touches `PostgresCDCIT.java`, `DorisErrorIT.java` - `31cf042b905a` "[Fix][E2E] Extend HANA startup readiness timeout" - touches `JdbcHanaIT.java` - `091f58e37f00` "[Fix][Zeta] Stabilize pending job cleanup regression test" - touches `CoordinatorServiceTest.java` - `1a7a9dd2626d` "[Fix][E2E] Stabilize Postgres CDC restore test timing" - touches `PostgresCDCIT.java`, `CoordinatorServiceTest.java` - `5eefa194acda` "[Chore] Refresh CI" (current head) - verified via `gh api repos/apache/seatunnel/commits/5eefa194acda --jq '.files | length'` -> `0`, a genuine empty CI retrigger. I diffed `d2bc272f3641...5eefa194acda` directly rather than trusting the commit subjects: the only four changed files across this round are `PostgresCDCIT.java`, `DorisErrorIT.java`, `JdbcHanaIT.java`, and `CoordinatorServiceTest.java`. None of `JdbcSinkAggregatedCommitter.java`, `XaGroupOpsImpl.java`, `XaFacadeImplAutoLoad.java`, or `GroupXaOperationResult.java` - the files this PR's actual fix lives in - changed at all this round. So this is not a from-scratch re-review; the core-logic conclusion from my 2026-08-30 comment stands unchanged. # What this round actually is All four substantive commits are CI-stability fixes for test flakes that earlier rounds on this exact PR had already identified as unrelated pre-existing flakiness (the Postgres CDC restore-timing race, the HANA five-minute tenant-DB startup window, and the coordinator pending-job-scheduler test's dependence on a synchronous `await().untilAsserted(Mockito.verify(...))` poll instead of a deterministic latch), not new work on the XA commit-failure fix itself. I spot-checked each patch instead of assuming the message matches the diff: - `PostgresCDCIT` (`7a951674dafb`, `1a7a9dd2626d`): moves the row insert for the restored job to after `waitForReplicationSlotActive(...)` and adds an explicit wait for the old replication slot to go inactive before treating the restored one as the current state. This removes a real race (inserting before the restored slot could consume it) rather than loosening any assertion - the `await().untilAsserted(...)` checks are unchanged in strictness, only the ordering/timing around them changed. - `JdbcHanaIT` (`31cf042b905a`): raises the container startup timeout from 5 to 10 minutes with a comment explaining HANA creates its tenant database after the process starts. A timeout increase for a genuinely slow, non-deterministic external dependency; the assertion being waited on (`Startup finished!` log line) is unchanged. - `CoordinatorServiceTest` (`091f58e37f00`, `1a7a9dd2626d`): replaces a `Mockito.verify(..., atLeastOnce())` polled via `await()` with a `CountDownLatch` that the mocked `preApplyResources()` call counts down itself, and runs the scheduler on the test's own executor (then, in the second commit, swaps in the production `executorService` field directly) instead of a detached thread. This is a strictly more deterministic synchronization primitive replacing a race-prone poll, not a weaker check - the final assertions (`pendingJobQueue` no longer contains the job, `interrupt()` was called) are byte-for-byte the same as before. - `DorisErrorIT` (`7a951674dafb`): the one change worth flagging on its own merits even though it's out of this PR's scope - it swaps an assertion on a specific stack-trace substring (`...RecordBuffer.checkErrorMessageByStreamLoad`) for one on `DorisConnectorErrorCode.STREAM_LOAD_FAILED.getDescription()`, while keeping the existing `getCode()` assertion and the non-zero exit code check untouched. That's trading a brittle white-box check (an internal method name that any unrelated refactor could rename) for a still-specific check on the connector's own public error-code description - I don't read this as a coverage reduction, but it's tangential to this PR and outside what I fixed here, so I'm noting it rather than owning it as part of this round's review. None of this round's commits touch JDBC, XA, or this PR's checkpoint/restore reconciliation logic, and none of it weakens an assertion, timeout direction, or coverage scope - it tightens timing determinism in every case I checked. # CI Dereferenced the apache `Build` pointer on the current head (`5eefa194acda`) to the real fork run: `DanielLeens/seatunnel` run `33355857983`. Unlike the last two rounds where I flagged failing job buckets before logs were retrievable, this run is progressing cleanly so far, about 15 minutes in: `changes`, `Sanity check results`, `License header`, `Dead links`, `Code style`, and `Check Helm Chart` have all completed successfully; the large integration-test matrix (`all-connectors-it-1` through `-8`, `engine-v2-it`, `transform-v2-it-part-1/2`, `unit-test` on both JDKs/OSes) is `in_progress`; `jdbc-connectors-it-part-1` and `all-connectors-it-2`/`-3` are still `queued` waiting for a runner slot. Zero failures on this head so far - a meaningfully cleaner start than the three early failures I saw and couldn't yet diagnose on the previous head (`d2bc272f3641`). # Process status (unchanged from my last comment) `reviewDecision` is still `CHANGES_REQUESTED`, still driven solely by @nzw921rx's 2026-07-27 review, which - as I 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. That review has not been revisited, updated, or dismissed since I last flagged it, and I cannot do so myself as the PR author. `mergeable_state` is still `blocked` for the same reason. The two low-severity carryovers from @li3zhi4 (PR description wording on the all-absent case, the still-unused `ignoreUnknown=true` path) also remain open and unaddressed this round. # Conclusion No change to my 2026-08-30 merge recommendation: **Ready to merge** on source correctness once CI finishes green. The only blockers are process, not code: @nzw921rx's stale `CHANGES_REQUESTED` needs a maintainer with write access to revisit or dismiss it, and this run's CI needs to finish (it's healthy so far, no need to intervene). -- 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]
