michaeljmarshall commented on a change in pull request #9393:
URL: https://github.com/apache/pulsar/pull/9393#discussion_r573437508
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java
##########
@@ -75,18 +75,17 @@ public LinuxBrokerHostUsageImpl(int
hostUsageCheckIntervalMin,
this.lastCollection = 0L;
this.usage = new SystemResourceUsage();
this.overrideBrokerNicSpeedGbps = overrideBrokerNicSpeedGbps;
- executorService.scheduleAtFixedRate(this::calculateBrokerHostUsage, 0,
- hostUsageCheckIntervalMin, TimeUnit.MINUTES);
boolean isCGroupsEnabled = false;
try {
isCGroupsEnabled =
Files.exists(Paths.get(CGROUPS_CPU_USAGE_PATH));
} catch (Exception e) {
log.warn("Failed to check cgroup CPU usage file: {}",
e.getMessage());
}
-
this.isCGroupsEnabled = isCGroupsEnabled;
- calculateBrokerHostUsage();
Review comment:
Great point. I agree that we shouldn't return until the class is fully
initialized. I'm guessing that was the original intent of including that method
call there, too. I can update the constructor as you suggested, and I think it
likely makes the most sense to cherry-pick your commits.
I also noticed that there is a race condition in the
`GenericBrokerHostUsageImpl`, where two different scheduled methods make calls
that update `cpuUsageSum` and `cpuUsageCount`. I'm wondering if we should add
logic to `synchronize` on one of the variables? If you have another suggestion,
happy to look into it.
----------------------------------------------------------------
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]
