DanielLeens commented on PR #12381: URL: https://github.com/apache/seatunnel/pull/12381#issuecomment-5846318068
Thanks for the thorough pass, and for pushing past the ceiling-widening framing to the actual failure mechanism — I checked it against the current source rather than taking the writeup at face value, and issue 1 holds up exactly as described. On issue 1: conceded, without reservation. `PayPalClient.execute()` (lines 189-190) returns on a transient status without reading the entity, and the `finally` block (lines 214-216) aborts unconditionally right after. `serve()` only counts `arrived` down after the body write succeeds (lines 194-195), guarded by a swallowing `catch (IOException ignored)` at line 199, and `closeWakesRetryWait`'s plain 503 reply takes exactly that branch. A latch that can be silently skipped isn't something a larger timeout can fix, and there's now independent confirmation of this from a different angle: a 50-run/20-run local experiment posted on this thread forces the exact IOException you describe and reproduces the failure 20/20 on this code, 0/20 against a version that counts the latch down earlier. My original approval treated the widening as purely a ceiling change with no correctness risk because the behavior-verifying bounds stay untouched — that part is still true, but it missed that the * arrival* signal itself can be swallowed independent of any bound. I'm walking that back for the PayPal half specifically. Practically: there's already an open PR, #12444, that fixes this at the root (counting `arrived` down before or independent of the body write, using the same `bodyless`-style pattern this file already has for other replies), rather than widening the wait around a latch that might never fire. Given that, I'd support dropping the `PayPalClientTest` changes from this PR entirely and letting #12444 carry that fix, rather than asking this PR to re-implement the same fix in place. On issues 2-5: I read your writeup on the `FileCollectReaderBehaviorTest` findings as describing races that predate this diff — issue 2 explicitly as "a second, independent failure mode the same test still has after the change," and issue 5 as a diagnostics gap in how `untilAsserted` reports failure, not a correctness regression this PR introduces. All the strict assertions in that test stay byte-for-byte unchanged, so I don't read those as blocking the ceiling widening itself, though they're worth their own follow-up given how much attention this test's flakiness is already getting. Issue 3's coupling between `ARRIVAL_WAIT_SECONDS` and the client's default 30s `request_timeout_ms` is a fair documentation gap on the current PayPal constant, and issue 4's fail-fast suggestion is a good diagnostic improvement independent of where the root-cause fix for issue 1 ultimately lands. Net: I agree with your CHANGES_REQUESTED on the PayPal half as it stands in this PR, and think the cleanest path is to let #12444 absorb that fix and have this PR keep only the `FileCollectReaderBehaviorTest` widening, with issues 2-5 tracked as separate follow-ups rather than blockers on the timing-ceiling change itself. -- 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]
