[GitHub] [hbase] Apache9 commented on a change in pull request #3710: HBASE-26308 Sum of multiplier of cost functions is not populated properly when we have a shortcut for trigger

[GitBox]

Apache9 commented on a change in pull request #3710:
URL: https://github.com/apache/hbase/pull/3710#discussion_r720343384



##########
File path: 
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -345,33 +346,27 @@ boolean needsBalance(TableName tableName, 
BalancerClusterState cluster) {
       return false;
     }
     if (areSomeRegionReplicasColocated(cluster)) {
-      LOG.info("Running balancer because at least one server hosts replicas of 
the same region.");
+      LOG.info("Running balancer because at least one server hosts replicas of 
the same region." +
+        " function cost={}", functionCost());

Review comment:
       This is a info level log, will the functionCost call cause performane 
issue?

##########
File path: 
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -345,33 +346,27 @@ boolean needsBalance(TableName tableName, 
BalancerClusterState cluster) {
       return false;
     }
     if (areSomeRegionReplicasColocated(cluster)) {
-      LOG.info("Running balancer because at least one server hosts replicas of 
the same region.");
+      LOG.info("Running balancer because at least one server hosts replicas of 
the same region." +
+        " function cost={}", functionCost());
       return true;
     }
 
     if (idleRegionServerExist(cluster)){
-      LOG.info("Running balancer because cluster has idle server(s).");
+      LOG.info("Running balancer because at least one server hosts replicas of 
the same region." +
+        "regionReplicaRackCostFunction={}", 
regionReplicaRackCostFunction.cost());
+      LOG.info("Running balancer because cluster has idle server(s)."+
+        " function cost={}", functionCost());
       return true;
     }
 
-    sumMultiplier = 0.0f;
     double total = 0.0;
     for (CostFunction c : costFunctions) {
-      float multiplier = c.getMultiplier();
-      double cost = c.cost();
       if (!c.isNeeded()) {
         LOG.trace("{} not needed", c.getClass().getSimpleName());
         continue;
       }
-      total += cost * multiplier;
-      sumMultiplier += multiplier;
+      total += c.cost() * c.getMultiplier();
     }
-    if (sumMultiplier <= 0) {
-      LOG.error("At least one cost function needs a multiplier > 0. For 
example, set "
-        + "hbase.master.balancer.stochastic.regionCountCost to a positive 
value or default");
-      return false;
-    }
-
     boolean balanced = (total / sumMultiplier < minCostNeedBalance);

Review comment:
       We do not update sumMultiplier but use it directly? It could be zero 
here?

##########
File path: 
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -598,8 +605,8 @@ private String functionCost() {
         builder.append(", ");
         double cost = c.cost();
         builder.append("imbalance=" + cost);
-        if (cost < minCostNeedBalance) {
-          builder.append(", balanced");
+        if (cost >= minCostNeedBalance) {
+          builder.append(", need balance");

Review comment:
       Why this change?




-- 
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: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org