michaeljmarshall commented on pull request #9393: URL: https://github.com/apache/pulsar/pull/9393#issuecomment-775720402
> One detail that immediately caught my eye in LinuxBrokerHostUsageImpl is the usage of System.currentTimeMillis(). Replacing that with System.nanoTime() could be helpful. The calculations that mix doubles and longs seem error prone. In looking at the class and the scheduling of the `calculateBrokerHostUsage` method, I can see that this class (and `GenericBrokerHostUsageImpl `) are not really designed to be called in immediate succession. (Side note, that makes me wonder about the choice to use `scheduleAtFixedRate` instead of `scheduleWithFixedDelay`.) Because we don't intend to call the methods in immediate succession, millis look like they could be fine, but I could definitely see making a switch to nanos too. What do you think? I also agree that the mix of longs and doubles seems error prone. > It would be useful to check how good the test coverage for this class is. These classes do seem pretty fundamental to load balancing and I believe they are also essential to the load shedding mechanisms, which are pretty important. There are tests that implicitly cover this code, but perhaps we can add something more explicit in another PR. It's likely that these tests aren't covered because some of the logic is hard to test. Reworking some of the classes to make them more testable might be necessary. > The code in LinuxBrokerHostUsageImpl rings some bells. I faced some issues earlier where the method to read the Linux /proc and /sys/fs files with Files.readAllBytes was causing odd issues. I'll have to check if I reported an issue back then or brought it up on Pulsar Slack etc. Let me know if you found anything regarding this issue. My most recent commit should definitely add some stability to the `LinuxBrokerHostUsageImpl` by not calling it at nearly the same time. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
