DanielLeens commented on PR #11602:
URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5696047739
@SEZ9 On your ask #2, I traced it through the code on `a283df0d52` rather
than answering from inspection alone, since it's worth getting right.
There are actually two separate "realtime" code paths here, and they agree
on *what* they do for a departed member but not on whether they log it:
- `JobMaster.getCurrJobMetrics(Map, failOnIncompleteResult)` is now shared
by both the realtime path (`failOnIncompleteResult=false`, reached via the
public `getCurrJobMetrics()`) and the terminal path
(`failOnIncompleteResult=true`, reached via the new private
`getFinalJobMetrics()`). The departed-member check sits *above* the
`failOnIncompleteResult` branch — `if
(nodeEngine.getClusterService().getMember(address) == null) {
LOGGER.warning(...); continue; }` — so realtime and terminal collection through
`JobMaster` skip-and-log a departed worker identically. That's the part this PR
actually unifies, and it's consistent.
- `CoordinatorService.getRunningJobMetrics()` (the cluster-wide, all-jobs
aggregator) doesn't go through `JobMaster.getCurrJobMetrics()` at all — it's a
separate path via `getRunningJobRawMetrics()` → `fetchRawMetricsFromWorker()`,
which does `if (nodeEngine.getClusterService().getMember(address) == null) {
return null; }` with no log call. Functionally identical (skip, no retry, no
exception), but silent instead of logged.
That asymmetry isn't new in this PR, though — the pre-PR code in
`fetchRawMetricsFromWorker`'s predecessor had the same unconditional `if
(member != null) { ... }` guard with no else-branch, so the "silent" half is
pre-existing behavior this PR didn't touch, not a regression it introduced. I
flag it only because you asked for confirmation of symmetry: both realtime
paths already treat a departed member the same functional way as the terminal
path (skip, don't spend retry budget on it), they just don't log it the same
way. Not something I'd hold this PR on — the part this PR actually changes (the
`JobMaster` realtime/terminal split) is correctly log-consistent. A follow-up
to add the same warning in `fetchRawMetricsFromWorker` would be a reasonable,
separate, low-risk cleanup if anyone wants full log parity.
Nothing else from me on `a283df0d52b1`. Still with you on waiting for the
#12311 rebase before judging CI.
--
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]
