[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #9393: Fix testBrokerSelectionForAntiAffinityGroup by increasing OverloadedThreshold

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #9393:
URL: https://github.com/apache/pulsar/pull/9393#discussion_r572629004



##########
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();
+
+        executorService.scheduleAtFixedRate(this::calculateBrokerHostUsage, 0,

Review comment:
       @lhotari - because we were scheduling `calculateBrokerHostUsage` with a delay of `0` minutes and then calling it only a few lines later, it is quite possible that there could have been a race internally that led to a calculated `elapsedSeconds` of `0`. If `elapsedSeconds` is `0`, we'll divide by by `0` in the method to calculate `cpuUsage`, which could likely be our culprit for the "Infinite" cpu usage you saw in your test.
   
   Here is the full method:
   
   https://github.com/apache/pulsar/blob/ce14ec4d0490cdad8fcc1146bb4d4a1b6cbd5979/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LinuxBrokerHostUsageImpl.java#L96-L127
   
   Also, note that this method likely should not have been called before setting `isCGroupsEnabled` because the internals of the method rely on that value. Perhaps @merlimat can provide a bit more context for whether or not this update adds value here (he added the `isCGroupsEnabled` code this past July).




----------------------------------------------------------------
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:
users@infra.apache.org