SEZ9 commented on PR #12454: URL: https://github.com/apache/seatunnel/pull/12454#issuecomment-5987793718
## Head updated to `ba304980c71f` — please re-review, the previous approval no longer applies This push changes the reviewed head, which I know is the opposite of the stability you asked for before merge. I am doing it anyway because it discharges a commitment I made on this thread on 2026-09-26, and the condition it was waiting on has now been met: > I agree that the smaller fixes should take precedence when they land. [...] once those fixes are in `dev`, I will rebase and remove the superseded checkpoint accessor/test and PayPal hunks, then rerun validation. **#12444 merged on 2026-09-28** (`426e44f96`). Leaving a now-redundant local fixture in a PR after its root-cause fix landed is exactly what I said I would not do, so I would rather pay for a re-review than merge it. ### What changed, and nothing else 1. **Reverted the `PayPalClientTest#closeWakesRetryWait` fixture** to the upstream version. #12444 fixes the flake in the embedded server helper — it releases the arrival latch *before* the body write and again unconditionally in a `finally` block, so the latch is released whichever branch fails. My version only avoided the body write inside this one test by marking the 503 reply bodyless. @goutamadwant measured the two as equivalent (`dev` 5/50 on JDK 11, each fix 0/50; with a forced `IOException` on the body write, `dev` 20/20 and both fixes 0/20). With the root-cause fix in `dev` the local fixture no longer earns its keep and only narrows what the test exercises. 2. **Rebased onto current `dev`** (`eb2eb1ddc`, 56 commits). No conflicts. Diff is now **18 files, +1224/-30** (was 19 files). `git diff upstream/dev HEAD -- connector-http-paypal/` is empty, i.e. the superseded hunk is gone and `dev`'s #12444 fix is what remains. ### Overlap status, re-checked against `dev` rather than from memory | PR | State | Effect here | |---|---|---| | #12449 | **MERGED** `146a1b5c5` | Already in this PR's previous base. The remaining `CheckpointCoordinatorFailoverIT` hunk is **not** a duplicate of it — it is two new lines asserting `cancelJob()` resolves to `CANCELED` through `waitForJobComplete()`, which is what exercises the `PhysicalVertex.cancel()` guard in this PR. Kept. | | #12444 | **MERGED** `426e44f96` | Hunk removed, as above. | | #12216 | **OPEN** | `TestFilterRowKindIT` assertion-writer `parallelism = 1` stays, with its existing source comment pointing at #12216. | | #12489 | **CLOSED** | Correcting my own earlier reference on this thread: I cited #12489 as the PR isolating the OceanBase duplicate-Debezium packaging fix. It is closed, so that `NotSerializableException: io.debezium.relational.TableId` failure is **not** currently tracked by an open PR. It remains out of scope here. | The `PayPalIT` e2e hunk (+4/-2) is unrelated to the flake — it adds a 503 OAuth retry leg and raises the expected OAuth call count from 2 to 3, i.e. new coverage for the bodyless-retry path. I left it as reviewed rather than widening this push. ### Answering the two open questions **Is this the final head?** `3080371ce733` is not; `ba304980c71f` is what I intend to be final. I have no further pushes planned, and I will not push again without saying so here first. **Did the full PostgreSQL suite pass on `3080371ce733`?** I can confirm less than that question asks for, so to be precise about what each piece of evidence covers: - Fork Build `36330123070` on head `3080371ce733` completed with conclusion **success** — I re-verified the head SHA on the run itself just now, not from my earlier comment. - The module-level **PostgreSQL 37 tests / 0 failures / 0 errors / 0 skips** figure I posted on 2026-09-27 was on head **`4f86cb0ac63e`**, not on `3080371ce733`. I did not independently confirm the per-module PostgreSQL numbers on `3080371ce733`, and I am not going to present the run-level `success` as if it were that. Since the head has moved, both are now superseded anyway. I will post the module-level PostgreSQL result from the CI run on `ba304980c71f` when it lands. ### Retracting one commitment I cannot keep On 2026-09-29 I said I would re-run @goutamadwant's stress setup against `3080371ce733` and post the numbers alongside the earlier ones. I should not have promised that: I do not have a JDK/Maven environment that can reproduce it, so any numbers I posted would not be mine. I am withdrawing that rather than leaving it hanging on the thread. The earlier stress numbers stand for the diffs they were run against, and the two fixes they compared (#12449, #12444) are now both merged into `dev`, which removes most of what those runs were deciding between. ### On the new PostgreSQL evidence @xinnyuli thanks — that is the clearest before/after on this PR's actual subject so far. **3/30 missing before, 0/28 after**, and more usefully the mechanism matches what the fix targets: in every missing run `id=15` starts exactly at the stored commit-end LSN and is filtered as already processed, and with this change the runs that hit the same boundary resume at the stored LSN and deliver the row. That is the transaction-boundary equality case `LsnOffsetTest` covers as a unit, observed end to end. The rebase did not touch any PostgreSQL file, so your evidence applies unchanged to `ba304980c71f`. CI is re-running on the new head. -- 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]
