SEZ9 commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5611737425
Thanks @DanielLeens — confirmed, we're fully aligned. To restate the agreed
scope so the next revision has one unambiguous checklist:
**Blocking for merge**
- **F1** — the hardcoded 3s timeout in `BaseService.java` drops a busy
member as an anonymous `{}`, which is indistinguishable from a genuinely empty
node and not configurable.
- **F2** — the per-member `invocationFuture.get(3, SECONDS)` sits inside the
same sequential `Stream.map()` lambda as `sendOperationToMemberNode`, so the
endpoint is still bounded by `3s * member count`. Your re-verification against
`88cc6b851818` matches what I saw. Fix direction as we both described: dispatch
all invocations first, collect the futures, then await them against a single
shared deadline derived from one `System.nanoTime()` start point (or
`CompletableFuture.allOf(...).get(3, SECONDS)` if adapted).
- **F4** — on `InterruptedException` the loop re-sets the interrupt flag and
keeps iterating, so remaining members fail instantly and silently and a 200 is
returned with partial data and no log line. Consolidating the await into one
place per F2 should fold this into a single interrupt-handling path.
- **Test coverage** — explicit tests for the timeout path, the interrupt
path, and the cancel path of the fan-out.
**Non-blocking follow-ups**
- **F3** — document the new `{}`-after-timeout behaviour for
`/system-monitoring-information` in the REST API docs (en/zh, v1/v2).
- **F5** — remove the duplicate `import java.util.concurrent.TimeUnit;` and
run Spotless on the test source so CI isn't blocked on formatting.
- **F6** — extend the locale test beyond the two static helpers to the
`renderLoad`/`renderOperationService` sites, and avoid mutating the JVM-global
default locale.
- **F7** — `invocationFuture.cancel(true)` doesn't stop the remote operation
and the `true` flag is a no-op on a Hazelcast `InvocationFuture`; either drop
the flag or document the intent.
Nothing further from my side. Once a new commit lands addressing the
blocking set I'll re-review the full diff alongside you.
<!-- streview-comment:928 -->
--
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]
