SEZ9 commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5578113294
Thanks @DanielLeens — agreed on all points, and the trace you did against
`88cc6b851818` matches what I saw: `sendOperationToMemberNode` and
`invocationFuture.get(3, SECONDS)` sit inside the same lambda in
`members.stream().map(...)` (BaseService.java:1407-1420), so the next member's
RPC isn't dispatched until the previous one returns or times out. The endpoint
is therefore still worst-case `3s × member count`, which is exactly what
PR12161-F2 describes and why the "bound REST fanout" claim doesn't hold yet.
Dispatch-all-then-await against a shared deadline
(`CompletableFuture.allOf(...).get(3, SECONDS)` or equivalent) is the fix I'd
like to see too.
Also agreed that F4 compounds F2: once one member throws
`InterruptedException`, the loop re-sets the flag and keeps iterating, so every
remaining member fails instantly and the caller gets a 200 with partial data
and nothing logged. Once the fan-out is parallel this becomes a single
interrupt-handling path, which should make it simpler to handle correctly
(stop, log, and don't render the failure as an empty node).
I'm fine treating F1, F2 and F4 plus the test-coverage gap you raised on the
timeout/interrupt/cancel branch as the blocking set for this round, with F3,
F5, F6 and F7 as the mechanical follow-ups.
Concrete asks for the next revision:
1. F2: fire all member invocations first, then await them against one shared
deadline.
2. F4: on `InterruptedException`, stop the fan-out, log it, and don't return
a silent partial 200.
3. F1: make the timeout configurable and render a timed-out member
distinguishably rather than as an anonymous `{}`.
4. Add tests covering the timeout, interrupt and cancel branches.
5. F3: document the timeout behaviour of `/system-monitoring-information` in
the REST API docs (en/zh, v1/v2).
6. F5: remove the duplicate `import java.util.concurrent.TimeUnit;` and run
Spotless on the test source.
7. F6: cover the `renderLoad`/`renderOperationService` sites in
`SeaTunnelHealthMonitorTest` and avoid mutating the JVM-global default locale.
8. F7: drop or justify `invocationFuture.cancel(true)`, since the `true`
flag has no effect on a Hazelcast `InvocationFuture` and doesn't stop the
remote operation.
Happy to re-review once those land.
<!-- streview-comment:883 -->
--
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]
