[GitHub] [hbase] ndimiduk commented on a change in pull request #3415: HBASE-25739 TableSkewCostFunction need to use aggregated deviation

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

ndimiduk commented on a change in pull request #3415:
URL: https://github.com/apache/hbase/pull/3415#discussion_r658136085



##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/DoubleArrayCost.java
##########
@@ -106,4 +88,32 @@ private static double getSum(double[] stats) {
     }
     return total;
   }
+
+  /**
+   * Return the min skew of distribution
+   */
+  public static double getMinSkew(double total, double numServers) {

Review comment:
       Why are the input arguments `double`? Can there be a fractional amount of either of these quantities?

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java
##########
@@ -301,17 +307,28 @@ public String getRack(ServerName server) {
     for (int i = 0; i < regionIndexToServerIndex.length; i++) {
       if (regionIndexToServerIndex[i] >= 0) {
         numRegionsPerServerPerTable[regionIndexToServerIndex[i]][regionIndexToTableIndex[i]]++;
+        numRegionsPerTable[regionIndexToTableIndex[i]]++;
       }
     }
 
-    numMaxRegionsPerTable = new int[numTables];
+    // Avoid repeated computation for planning
+    meanRegionsPerTable = new double[numTables];
+    regionSkewByTable = new double[numTables];
+    maxRegionSkewByTable  = new double[numTables];
+    minRegionSkewByTable = new double[numTables];
+
+    for (int i = 0; i < numTables; i++) {
+      meanRegionsPerTable[i] = Double.valueOf(numRegionsPerTable[i]) / numServers;
+      minRegionSkewByTable[i] += DoubleArrayCost.getMinSkew(numRegionsPerTable[i], numServers);
+      maxRegionSkewByTable[i] += DoubleArrayCost.getMaxSkew(numRegionsPerTable[i], numServers);
+    }
+
     for (int[] aNumRegionsPerServerPerTable : numRegionsPerServerPerTable) {
-      for (tableIndex = 0; tableIndex < aNumRegionsPerServerPerTable.length; tableIndex++) {
-        if (aNumRegionsPerServerPerTable[tableIndex] > numMaxRegionsPerTable[tableIndex]) {
-          numMaxRegionsPerTable[tableIndex] = aNumRegionsPerServerPerTable[tableIndex];
-        }
+      for (int tableIdx = 0; tableIdx < aNumRegionsPerServerPerTable.length; tableIdx++) {
+        regionSkewByTable[tableIdx] += Math.abs(aNumRegionsPerServerPerTable[tableIdx]
+          - meanRegionsPerTable[tableIdx]);
       }
-    }
+     }

Review comment:
       nit: white space.

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/DoubleArrayCost.java
##########
@@ -106,4 +88,32 @@ private static double getSum(double[] stats) {
     }
     return total;
   }
+
+  /**
+   * Return the min skew of distribution
+   */
+  public static double getMinSkew(double total, double numServers) {

Review comment:
       Is `total` the "total number of regions in the cluster"?

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/DoubleArrayCost.java
##########
@@ -106,4 +88,32 @@ private static double getSum(double[] stats) {
     }
     return total;
   }
+
+  /**
+   * Return the min skew of distribution
+   */
+  public static double getMinSkew(double total, double numServers) {
+    double mean = total / numServers;
+    // It's possible that there aren't enough regions to go around
+    double min;
+    if (numServers > total) {

Review comment:
       should this be `>=` ?

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -240,7 +240,8 @@ protected void loadConf(Configuration conf) {
     curFunctionCosts = new double[costFunctions.size()];
     tempFunctionCosts = new double[costFunctions.size()];
 
-    LOG.info("Loaded config; maxSteps=" + maxSteps + ", stepsPerRegion=" + stepsPerRegion +
+    LOG.info("Loaded config; maxSteps=" + maxSteps + " ,runMaxSteps=" + runMaxSteps,

Review comment:
       nit: white space.

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CostFunction.java
##########
@@ -89,13 +89,14 @@ protected void regionMoved(int region, int oldServer, int newServer) {
    * @return The scaled value.
    */
   protected static double scale(double min, double max, double value) {
-    if (max <= min || value <= min) {
+    if (max <= min || value <= min
+      || Math.abs(max - min) <= 0.01 || Math.abs(value - min) <= 0.01) {

Review comment:
       values like `minCostNeedBalance` below are at 0.00 precision, so I think we should have an epsilon of at least 0.000 precision.




-- 
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