DanielLeens commented on PR #12161: URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5569391373
+1 on this, @SEZ9 — good extension of what I flagged. Issue 2 is the one I missed: I focused on whether the per-member wait was bounded (it is, via `invocationFuture.get(3, SECONDS)`) but didn't trace that `members.stream().map(...)` (BaseService.java:1407-1420) dispatches `sendOperationToMemberNode` and blocks on that same member's future inside one lambda before moving to the next element — so the next member's RPC isn't even sent until the previous one returns or times out. I confirmed that against the current head (`88cc6b851818`): the send and the `get()` are both inside the same stream element. That means the endpoint is still worst-case `3s × member count`, which undercuts the PR title's "bound REST fanout" claim more directly than what I caught (I only flagged the missing test coverage and the non-propagating `cancel(true)`, not the sequencing itself). Dispatch-all-then-await-against-a-shared-deadline (or `CompletableFuture.allOf(...).get(3, SECONDS)`) is the right fi x. Issue 4 is also a solid catch and compounds Issue 2: once one member throws `InterruptedException`, the flag stays set for the rest of the still-sequential stream, so every remaining member fails instantly and silently with nothing logged — worth calling out as its own item alongside the High-severity test-coverage gap I raised. Combining our two passes, I'd treat your Issues 1, 2 and 4 together with my Issue 3 (zero test coverage on the timeout/interrupt/cancel branch) as the blocking set for this round; the Spotless/doc/locale-test items (your 3, 5, 6, my 1) are straightforward mechanical fixes. Thanks for the thorough pass — this needs another revision before merge. -- 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]
