You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@storm.apache.org by "mwkang (via GitHub)" <gi...@apache.org> on 2026/06/12 02:31:29 UTC
[PR] Skip stale worker heartbeats from orphaned worker directories (storm)
mwkang opened a new pull request, #8789:
URL: https://github.com/apache/storm/pull/8789
## What is the purpose of the change
Fixes #8786.
The supervisor builds the heartbeat batch it sends to Nimbus from every worker directory present on disk (`SupervisorUtils.readWorkerHeartbeats` → `ReportWorkerHeartbeats.getSupervisorWorkerHeartbeatsFromLocal`), with no check on whether the worker is still alive. When a worker directory is left behind — e.g. a worker that died before the supervisor finished `Container.cleanUpForRestart()` — its frozen heartbeat keeps being reported on every reporting round. Orphaned directories are only reclaimed when `ReadClusterState` is constructed at supervisor startup, not from the periodic sync loop, so at runtime the stale heartbeat is reported indefinitely until the next supervisor restart.
Because the reported topology is usually no longer assigned (and its conf may already be deleted), Nimbus repeatedly tries to read the topology conf while processing that heartbeat and floods its log with `Exception when getting heartbeat timeout` / `NotAliveException`. STORM-4022 only suppressed the `NotAliveException` logging on the Nimbus side; the underlying behavior — the supervisor reporting heartbeats for dead/orphaned workers — was never addressed, and the generic-exception branch still logs.
This change filters stale heartbeats out in `ReportWorkerHeartbeats`: any heartbeat whose age exceeds `supervisor.worker.timeout.secs` is skipped and not forwarded to Nimbus. That is the same timeout `Slot` uses to decide a worker is dead, so a worker past it is already considered dead and should not be reported. A live worker always refreshes its heartbeat well within the timeout, so no valid heartbeat is ever dropped; a heartbeat exactly at the boundary (age == timeout) is still reported.
## How was the change tested
Added `ReportWorkerHeartbeatsTest` (driven by `Time.SimulatedTime`) covering `getSupervisorWorkerHeartbeatsFromLocal`:
- a fresh heartbeat is reported;
- a heartbeat exactly at the timeout boundary (age == timeout) is still reported;
- a heartbeat just past the timeout (age == timeout + 1) is filtered out;
- a long-stale orphaned heartbeat (a day old) is filtered out;
- null local heartbeats continue to be skipped.
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on code in PR #8789:
URL: https://github.com/apache/storm/pull/8789#discussion_r3417156161
##########
storm-server/src/main/java/org/apache/storm/daemon/supervisor/timer/ReportWorkerHeartbeats.java:
##########
@@ -59,7 +64,7 @@ private SupervisorWorkerHeartbeats getAndResetWorkerHeartbeats() {
}
}
- private SupervisorWorkerHeartbeats getSupervisorWorkerHeartbeatsFromLocal(Map<String, LSWorkerHeartbeat> localHeartbeats) {
Review Comment:
@mwkang apologies for not noting this earlier. We are relaxing the encapsulation of the class to allow easier tests. We can either add the VisibleForTesting annotation or we move the filtering logic to a separate class that we can then test independently (cleaner)
Ideally we should test ReporterWorkerHeartbeats as a whole through run(), but that will entail refactoring the class so we can inject mocks. Or we can add an integration test if it does not exist already. I don't want to expand the scope of your PR too much since this is an old class, though.
Let me know what you think. Cheers
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on code in PR #8789:
URL: https://github.com/apache/storm/pull/8789#discussion_r3419536934
##########
storm-server/src/main/java/org/apache/storm/daemon/supervisor/timer/ReportWorkerHeartbeats.java:
##########
@@ -59,7 +64,7 @@ private SupervisorWorkerHeartbeats getAndResetWorkerHeartbeats() {
}
}
- private SupervisorWorkerHeartbeats getSupervisorWorkerHeartbeatsFromLocal(Map<String, LSWorkerHeartbeat> localHeartbeats) {
Review Comment:
Lets go with the annotation. Thank you
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu merged PR #8789:
URL: https://github.com/apache/storm/pull/8789
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #8789:
URL: https://github.com/apache/storm/pull/8789#issuecomment-4702450309
@mwkang Would you mind rebasing this PR and updating regarding the tick fixes from https://github.com/apache/storm/pull/8788 ?
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "mwkang (via GitHub)" <gi...@apache.org>.
mwkang commented on PR #8789:
URL: https://github.com/apache/storm/pull/8789#issuecomment-4703997525
Thanks for the review! I've rebased onto master.
I moved it to the long-based clock, mirroring the pattern #8788 introduced in HeartbeatCache:
```
long hbAgeSecs = Time.deltaSecsLong(lsWorkerHeartbeat.get_time_secs());
```
The comparison semantics are unchanged (boundary age == timeout is still reported). The unit test was aligned to the long clock (Time.currentTimeSecsLong()) too.
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #8789:
URL: https://github.com/apache/storm/pull/8789#issuecomment-4689926334
Thx for the PR. Think we should get https://github.com/apache/storm/pull/8788 in first, so we can adjust the heartbeat getters.
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "mwkang (via GitHub)" <gi...@apache.org>.
mwkang commented on code in PR #8789:
URL: https://github.com/apache/storm/pull/8789#discussion_r3417459783
##########
storm-server/src/main/java/org/apache/storm/daemon/supervisor/timer/ReportWorkerHeartbeats.java:
##########
@@ -59,7 +64,7 @@ private SupervisorWorkerHeartbeats getAndResetWorkerHeartbeats() {
}
}
- private SupervisorWorkerHeartbeats getSupervisorWorkerHeartbeatsFromLocal(Map<String, LSWorkerHeartbeat> localHeartbeats) {
Review Comment:
Thanks @reiabreu, good catch on the encapsulation.
I went with `@VisibleForTesting` here, mainly to keep the change minimal as you suggested since this is an old class.
My reasoning: the filtering logic itself is quite simple (`hbAgeSecs` > `workerTimeoutSecs`), so pulling just the predicate into its own class felt like overkill. By testing `getSupervisorWorkerHeartbeatsFromLocal` directly, we can exercise the whole pipeline in a single unit test—including the staleness filter, boundary semantics (e.g., age == timeout), null-heartbeat skipping, and the mapping to `SupervisorWorkerHeartbeat` (storm id / supervisor id). This felt like more valuable and comprehensive coverage to lock in, which is why I opted to relax the visibility for now.
That said, if you'd prefer not to relax the visibility at all, I'm more than happy to extract the entire `localHeartbeats` -> `SupervisorWorkerHeartbeats` transformation into a stateless helper class. This would allow us to test the logic independently and maintain the same test coverage without compromising encapsulation.
Let me know which approach you prefer. Cheers!
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip stale worker heartbeats from orphaned worker directories (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on PR #8789:
URL: https://github.com/apache/storm/pull/8789#issuecomment-4702334988
> Thx for the PR. Think we should get #8788 in first, so we can adjust the heartbeat getters.
#8788 has been merged
--
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: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org