DanielLeens commented on PR #12454: URL: https://github.com/apache/seatunnel/pull/12454#issuecomment-5846337325
Thanks both — this is a genuinely useful thread to have before merge, not after. @goutamadwant, the methodology is solid (same `dev` base, fresh JVM per run, `-XX:ActiveProcessorCount=2` on JDK 11 to reproduce the real flake conditions) and I don't have anything to add to your numbers. I did go back and diff each of the three hunks against #12449/#12444/#12216 myself before answering, though, and I land in a slightly different place on two of them: **Checkpoint accessor (`CheckpointCoordinatorFailoverIT`) — I'd keep this PR's version, not #12449's.** #12449 synchronizes the fault injection by reflecting into `CheckpointCoordinator`'s private `lock` and `pendingCounter` fields (`ReflectionUtils.getField(coordinator, "lock" / "pendingCounter")`). That's exactly the pattern my first review of this PR flagged as fragile (Issue 4) — and SEZ9 fixed it here by adding the small `@VisibleForTesting runWhenNoCheckpointPending(Runnable)` accessor, which I confirmed still holds the coordinator's real trigger lock and closes that issue cleanly. Dropping this PR's hunk in favor of #12449 would trade a proper accessor for raw reflection, which is a step backward on maintainability even though both are green today. If anything, I'd rather see #12449 adopt this PR's accessor than the reverse. **PayPal (`PayPalClientTest`) — real overlap, but I'd treat it like the FilterRowKind case rather than dropping it outright.** #12454's fix (a `bodyless` flag on one `Reply`) and #12444's fix (reordering the shared `serve()` helper's latch countdown, plus an unconditional `finally`) are different mechanisms for the same flake, not literal duplicates, and neither is merged yet. If #12454 lands first with the hunk removed, the flake comes back until #12444 also lands. I'd suggest keeping it, with a one-line comment pointing at #12444 as the more general fix — same treatment as the FilterRowKind stopgap. **FilterRowKind and the recovery/CDC-LSN core — agreed as proposed.** `parallelism = 1` as a stopgap with a comment pointing at #12216, and the `TaskExecutionService`/`PhysicalVertex` recovery fix plus the Postgres CDC LSN backport as the PR's real payload, description retitled to match. My APPROVED review already covers that core on its technical merits and doesn't need to be redone for it. On the last point, SEZ9 — I don't currently have a savepoint-restore repro on hand where a commit and the next transaction's first event share a WAL LSN, so I can't run that before/after comparison myself right now. Agreed it would close the last gap in the evidence; not a blocker on my end. So: no action needed on the checkpoint-accessor hunk, a small comment addition (not a removal) on the PayPal hunk once someone has a moment, and the description update whenever it's convenient. Happy to take a quick look once the comment/description touch-ups are in. === END REPLY === -- 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]
