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