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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
