DanielLeens commented on PR #12313: URL: https://github.com/apache/seatunnel/pull/12313#issuecomment-5846314203
Thanks for actually running this in a loop, @goutamadwant — that's more signal than I had. Reading it the same way you do: 0/16, 0/16, 0/15 across dev, this PR, and #12316 tells us the resized fixture doesn't regress anything under either change, and nothing more. It's not evidence the fixture fixes the CI starvation, since none of us have reproduced that starvation locally yet. Agreed on the cadence too — @SEPURI-SAI-KRISHNA's read was right and I traced it the same way: the checkpoint lock is held for the full ~8s it takes to drain a 4000-row split at the sink's throttled ~500 rows/sec, so the free window recurs roughly every 9s, not every 1s. Real margin against `checkpoint.interval = 15000` is ~1.7x, not the 15x-90x the current Javadoc/`.conf` comments imply. I'll push a follow-up correcting that wording to state the per-split cadence and the actual margin, and note explicitly that `row.num`, `split.num`, `write_delay_ms`, and `checkpoint.interval` are coupled so a future edit to any one of them is made against the real budget, not the idealized one. On coverage, you're right and it's a good catch: with every split now completing in a single `pollNext` call, this test no longer exercises the case where the reader re-enters the checkpoint lock back-to-back across thousands of polls with no scheduled release point — which is exactly the engine-side behavior #12316 addresses. I'd like to keep that covered rather than lose it. Two things I want your read on before I add it: 1. Should the `split.num = 1` variant land in this PR but stay disabled (e.g. `@Disabled`) until #12316 merges, or would you rather it go in alongside #12316 itself so it's enabled together with the fix it's actually covering? Given the original workload is the one starving on `dev` today, I lean toward the latter, but I'd rather not guess. 2. If it lands here, do you have a preference between a separate test method in `BackpressureSlowSinkIT` versus a parameterized variant of the existing one? Separately, unrelated to either of your comments: the current CI failure on this head is `SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck`, a pre-existing dev-level flake tracked by #12311, which is still open and unmerged as of now. A sync wouldn't clear it — the right move once we're ready is just to retrigger the failing job, since `BackpressureSlowSinkIT` itself already passed cleanly in this same run. -- 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]
