DanielLeens commented on PR #12161:
URL: https://github.com/apache/seatunnel/pull/12161#issuecomment-5674313772
Thanks @SEZ9 — good catch that I hadn't re-checked the commit list closely
enough; the fan-out rework actually landed already, in `93e11ea7f0a1`
(2026-09-09T13:11:39Z, right after my last comment), so I went and read that
diff directly against `BaseService.java` rather than taking the commit message
on faith. Confirming each point:
- **F2 (serial fan-out)** — resolved.
`getSystemMonitoringInformationJsonValues()` now dispatches
`sendOperationToMemberNode` for every member into a `LinkedHashMap<Address,
InternalCompletableFuture<Object>>` first, then hands it to a new
`collectHealthMetrics(...)` that computes one `deadlineNanos` up front and
awaits each future for `Math.max(1, deadlineNanos - now)`. Total wait is now
bounded by the shared deadline, not `timeout * member count`.
- **F1 (anonymous `{}` on timeout, hardcoded timeout)** — resolved. The
deadline is now `seatunnel.engine.health-metrics-timeout-seconds` (new `Option`
in `ServerConfigOptions.MasterServerConfigOptions`, default 3s, wired through
`EngineConfig`). A member that times out, fails dispatch, or fails execution is
reported via `unfinishedMember(address, reason)` with `host`/`port`/`error`
(`timeout` / `dispatch-failure` / `execution-failure`) instead of an empty
object.
- **F4 (interrupt handling)** — resolved. `InterruptedException` now logs a
warning, restores the interrupt flag, and `break`s out of the collection loop
instead of continuing to iterate. One residual, non-blocking note: the members
not yet processed at that point are simply omitted from the response array
rather than getting their own marker entry (see
`testInterruptStopsWaitingForRemainingMembers`, which asserts 0 entries for 2
members) — still silent at the HTTP layer even though it's now logged
server-side. Low severity, fine as a follow-up rather than a blocker.
- **F5** — confirmed, the duplicate `TimeUnit` import is gone, and the
`Build` check on the current head is green, which includes Spotless.
- **F3** — confirmed, `docs/en(zh)/engines/zeta/rest-api-v1.md` and
`rest-api-v2.md` both got a new paragraph describing the shared deadline and
the `error` marker shape.
- **F7** — resolved with your suggested approach: `cancel(true)` became
`cancel(false)` plus an explicit comment that cancel only releases the local
future and can't stop the remote operation.
Test coverage: the new `BaseServiceHealthMetricsTest` covers timeout, the
shared-deadline bound, interrupt-stops-remaining, execution-failure and
dispatch-failure, all against directly-constructed `InternalCompletableFuture`s
with no `Thread.sleep`/wall-clock flakiness — rating this **Stable**.
With that verified, I don't have any remaining code-level blockers on my
side — F6 stays an accepted non-blocking follow-up as we agreed. CI is green on
`93e11ea7f0a1`. The PR still shows `mergeable_state: blocked`, which looks like
a branch-protection/review-count gate rather than a conflict or CI issue; since
I'm a comment-only reviewer here, that part is for you/a committer to clear.
From the code side, this is ready to 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]
