DanielLeens commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5578538435
Thanks @SEZ9 — I independently re-verified the sequencing claim against the
current head (`88cc6b851818`) and it checks out exactly as described.
`getSystemMonitoringInformationJsonValues()` (`BaseService.java:1404-1446`)
is:
```
members.stream() // line 1409 — plain
sequential stream, no .parallel()
.map(member -> {
...
invocationFuture = NodeEngineUtil.sendOperationToMemberNode(...);
// line 1417 — send
input = (String)
invocationFuture.get(HEALTH_METRICS_TIMEOUT_SECONDS, TimeUnit.SECONDS); //
line 1423 — blocking wait, same lambda
...
})
.collect(JsonArray::new, JsonArray::add, JsonArray::add); // line 1445
```
There's no `.parallel()` anywhere on this stream, and `Stream.map()` on a
sequential stream evaluates one element fully — including any blocking work
inside the mapping function — before advancing to the next. Since the send
(`sendOperationToMemberNode`) and the bounded wait (`invocationFuture.get(3,
SECONDS)`) both live inside that single per-member lambda, the RPC to member
N+1 is provably not issued until member N's future resolves, times out, or
throws. `HEALTH_METRICS_TIMEOUT_SECONDS = 3` (`BaseService.java:1449`), so
worst case is exactly `3s * member count` for this endpoint, confirming the
"bound REST fanout" claim in the PR title isn't yet true for cluster sizes
beyond one member.
Agreed this and F4 (the interrupt-flag issue on the same sequential loop)
belong in the blocking set alongside F1 and the test-coverage gap. On the fix
direction: dispatching all `sendOperationToMemberNode` calls first (collecting
the `InvocationFuture`s into a list) and only then awaiting them — either
individually against a shared deadline computed from a single
`System.nanoTime()` start point, or via `CompletableFuture.allOf(...).get(3,
SECONDS)` if the futures get adapted to `CompletableFuture` — removes the
per-member serialization entirely and naturally folds F4's interrupt handling
into one place instead of N copies inside the loop, same as you noted.
All 8 concrete asks look right and complete to me as the checklist for the
next revision; nothing to add or push back on. I'll re-review the full diff as
soon as a new commit lands.
--
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]
