[hbase] branch branch-2.2 updated: HBASE-23944 The method setClusterLoad of SimpleLoadBalancer is incorrect when balance by table (#1243)

[zg...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zghao pushed a commit to branch branch-2.2
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-2.2 by this push:
     new 6587a95  HBASE-23944 The method setClusterLoad of SimpleLoadBalancer is incorrect when balance by table (#1243)
6587a95 is described below

commit 6587a95eaf77584211247ab7d754c6c24c59682b
Author: nyl3532016 <ny...@163.com>
AuthorDate: Sat Mar 7 16:18:41 2020 +0800

    HBASE-23944 The method setClusterLoad of SimpleLoadBalancer is incorrect when balance by table (#1243)
    
    Signed-off-by: Guanghao Zhang <zg...@apache.org>
---
 .../hbase/master/balancer/SimpleLoadBalancer.java  | 17 ++++++++++------
 .../master/balancer/TestDefaultLoadBalancer.java   | 23 +++++++++++++++++-----
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java
index 89de13b..d3ccda4 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java
@@ -107,16 +107,21 @@ public class SimpleLoadBalancer extends BaseLoadBalancer {
   }
 
   @Override
-  public void setClusterLoad(Map<TableName, Map<ServerName, List<RegionInfo>>> clusterLoad){
+  public void setClusterLoad(Map<TableName, Map<ServerName, List<RegionInfo>>> clusterLoad) {
     serverLoadList = new ArrayList<>();
+    Map<ServerName, Integer> server2LoadMap = new HashMap<>();
     float sum = 0;
-    for(Map.Entry<TableName, Map<ServerName, List<RegionInfo>>> clusterEntry : clusterLoad.entrySet()){
-      for(Map.Entry<ServerName, List<RegionInfo>> entry : clusterEntry.getValue().entrySet()){
-        if(entry.getKey().equals(masterServerName)) continue; // we shouldn't include master as potential assignee
-        serverLoadList.add(new ServerAndLoad(entry.getKey(), entry.getValue().size()));
-        sum += entry.getValue().size();
+    for (Map.Entry<TableName, Map<ServerName, List<RegionInfo>>> clusterEntry : clusterLoad.entrySet()) {
+      for (Map.Entry<ServerName, List<RegionInfo>> entry : clusterEntry.getValue().entrySet()) {
+        if (entry.getKey().equals(masterServerName)) continue; // we shouldn't include master as potential assignee
+        int regionNum = entry.getValue().size();
+        server2LoadMap.compute(entry.getKey(), (k, v) -> v == null ? regionNum : regionNum + v);
+        sum += regionNum;
       }
     }
+    server2LoadMap.forEach((k, v) -> {
+      serverLoadList.add(new ServerAndLoad(k, v));
+    });
     avgLoadOverall = sum / serverLoadList.size();
   }
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java
index 0d8529b..8382722 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestDefaultLoadBalancer.java
@@ -171,20 +171,33 @@ public class TestDefaultLoadBalancer extends BalancerTestBase {
    */
   @Test
   public void testImpactOfBalanceClusterOverall() throws Exception {
+    testImpactOfBalanceClusterOverall(false);
+  }
+
+  @Test
+  public void testImpactOfBalanceClusterOverallWithClusterLoadPerTable() throws Exception {
+    testImpactOfBalanceClusterOverall(true);
+  }
+
+  private void testImpactOfBalanceClusterOverall(boolean useClusterLoadPerTable) throws Exception {
     Map<TableName, Map<ServerName, List<RegionInfo>>> clusterLoad = new TreeMap<>();
     Map<ServerName, List<RegionInfo>> clusterServers = mockUniformClusterServers(mockUniformCluster);
     List<ServerAndLoad> clusterList = convertToList(clusterServers);
     clusterLoad.put(TableName.valueOf(name.getMethodName()), clusterServers);
     // use overall can achieve both table and cluster level balance
-    HashMap<TableName, TreeMap<ServerName, List<RegionInfo>>> result1 = mockClusterServersWithTables(clusterServers);
-    loadBalancer.setClusterLoad(clusterLoad);
+    HashMap<TableName, TreeMap<ServerName, List<RegionInfo>>> clusterLoadPerTable = mockClusterServersWithTables(clusterServers);
+    if (useClusterLoadPerTable) {
+      loadBalancer.setClusterLoad((Map)clusterLoadPerTable);
+    } else {
+      loadBalancer.setClusterLoad(clusterLoad);
+    }
     List<RegionPlan> clusterplans1 = new ArrayList<RegionPlan>();
     List<Pair<TableName, Integer>> regionAmountList = new ArrayList<Pair<TableName, Integer>>();
-    for(TreeMap<ServerName, List<RegionInfo>> servers : result1.values()){
+    for (TreeMap<ServerName, List<RegionInfo>> servers : clusterLoadPerTable.values()) {
       List<ServerAndLoad> list = convertToList(servers);
       LOG.info("Mock Cluster : " + printMock(list) + " " + printStats(list));
       List<RegionPlan> partialplans = loadBalancer.balanceCluster(servers);
-      if(partialplans != null) clusterplans1.addAll(partialplans);
+      if (partialplans != null) clusterplans1.addAll(partialplans);
       List<ServerAndLoad> balancedClusterPerTable = reconcile(list, partialplans, servers);
       LOG.info("Mock Balance : " + printMock(balancedClusterPerTable));
       assertClusterAsBalanced(balancedClusterPerTable);
@@ -194,6 +207,6 @@ public class TestDefaultLoadBalancer extends BalancerTestBase {
       }
     }
     List<ServerAndLoad> balancedCluster1 = reconcile(clusterList, clusterplans1, clusterServers);
-    assertTrue(assertClusterOverallAsBalanced(balancedCluster1, result1.keySet().size()));
+    assertTrue(assertClusterOverallAsBalanced(balancedCluster1, clusterLoadPerTable.keySet().size()));
   }
 }