DanielLeens commented on PR #11602: URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5203570053
@SEZ9 thanks for the second pass on `16308107a1eb` — I re-checked it against my own review from a few hours earlier on the same head, and our conclusions converge. +1 on Issues 1, 3, and 5 in your latest review (hardcoded/duplicated 3s timeout, the silent fail-fast→best-effort semantic change on `getRunningJobMetrics()`, and the duplicated constant) — these are the same two root issues I listed as Issues 1 and 2. I also agree with your Issue 2 (the `SubPlan` retry predicate needs a comment explaining why `FinalMetricsCollectionException` is retryable) and Issue 4 (the public `getCurrJobMetrics(Map)` Javadoc should mention that an interrupted collection returns a truncated list, not just "missing individual workers") — both are small, correct asks I didn't call out and am happy to fold in. One piece of evidence to add on top of your Issue 1/3 discussion, since it affects how "fix the timeout" should actually be scoped: I traced `fetchTaskGroupMetrics`/`fetchRawMetricsFromWorker` down to `NodeEngineUtil.sendOperationToMemberNode()`, which builds the call via `createInvocationBuilder(...).setAsync().invoke()` — this returns Hazelcast's internal `InvocationFuture`, not a plain `CompletableFuture`. Calling `.get(3000, MILLISECONDS)` on that future and letting it throw `TimeoutException` does **not** cancel the underlying `Invocation` — the target worker keeps executing (or queuing) the `GetTaskGroupMetricsOperation`/`GetMetricsOperation` regardless, and the invocation stays registered on the master until the response finally arrives (now discarded) or Hazelcast's own internal call-timeout reclaims it (`hazelcast.operation.call.timeout.millis`, default 60s) — independent of this PR's 3s budget. So this isn't an unbounded leak (Hazelcast's own registry eventually reclaims it), but it does mean the bound only protects the master's calling thread, not the actual work done against a slow worker. Before this PR, the master thread stayed blocked on the one in-flight invocation per worker, which incidentally throttled how many concurrent requests a stuck worker could receive. After this PR, the master returns after 3s and is free to be polled again (e.g. by REST) while the prior invocation to that same worker is still alive in Hazelcast's registry — so a persistently slow (not dead) worker polled faster than it can respond can now accumulate multiple concurrently in-flight metrics operations against it, each still consuming worker-side CPU/heap until the ~60s internal reclaim fires. Worth a one-line callout alongside your Issue 1 fix (Option A/B) so operators don't assume lowering `METRICS_FETCH_TIMEOUT_MS` reduces load on the stuck worker — it only reduces how long the mas ter waits. Separately, a freshness note rather than a new issue: I pulled the fork's `Build` run for this exact head (`16308107a1eb`) just now and `Run / Code style` is still failing — same Spotless violation I flagged as my Issue 3 yesterday. It's not mentioned in your latest pass, so flagging in case it wasn't rechecked; it's still the cheapest fix on the list and it's currently blocking all other CI signal (unit-test / IT jobs are all skipped as a result). No pushback otherwise — our merge recommendations land in the same place (real correctness fixes from the previous round are solid; what's left is the timeout/config story, the doc gap, and getting CI green). -- 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]
