From 90f986497b58ef4107b8fdc5d07416efda355003 Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Wed, 5 May 2021 15:48:01 +0800 Subject: [PATCH] HBASE-25834 Remove balanceTable method from LoadBalancer interface (#3217) Signed-off-by: Yulin Niu --- .../hadoop/hbase/master/LoadBalancer.java | 18 ++------ .../master/balancer/BaseLoadBalancer.java | 41 ++++++++++++++++--- .../master/balancer/SimpleLoadBalancer.java | 22 ++++------ .../master/balancer/TestBaseLoadBalancer.java | 7 +--- .../favored/FavoredNodeLoadBalancer.java | 2 +- .../balancer/FavoredStochasticBalancer.java | 2 +- .../balancer/MaintenanceLoadBalancer.java | 6 --- .../balancer/StochasticLoadBalancer.java | 2 +- .../rsgroup/RSGroupBasedLoadBalancer.java | 37 +---------------- .../LoadBalancerPerformanceEvaluation.java | 9 ++-- 10 files changed, 58 insertions(+), 88 deletions(-) diff --git a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java index 491b5fabe42..a6f5357d379 100644 --- a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java +++ b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java @@ -72,25 +72,13 @@ public interface LoadBalancer extends Configurable, Stoppable, ConfigurationObse void setClusterInfoProvider(ClusterInfoProvider provider); /** - * Perform the major balance operation for cluster, will invoke {@link #balanceTable} to do actual - * balance. Normally not need override this method, except {@link SimpleLoadBalancer} and - * {@code RSGroupBasedLoadBalancer} + * Perform the major balance operation for cluster. * @param loadOfAllTable region load of servers for all table * @return a list of regions to be moved, including source and destination, or null if cluster is * already balanced */ - List balanceCluster(Map>> loadOfAllTable) throws IOException; - - /** - * Perform the major balance operation for table, all class implement of {@link LoadBalancer} - * should override this method - * @param tableName the table to be balanced - * @param loadOfOneTable region load of servers for the specific one table - * @return List of plans - */ - List balanceTable(TableName tableName, - Map> loadOfOneTable); + List balanceCluster(Map>> loadOfAllTable) + throws IOException; /** * Perform a Round Robin assignment of regions. diff --git a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java index 56fb96e6f9e..93a733126a1 100644 --- a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java +++ b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java @@ -610,13 +610,44 @@ public abstract class BaseLoadBalancer implements LoadBalancer { return returnMap; } - @Override - public abstract List balanceTable(TableName tableName, - Map> loadOfOneTable); + /** + * Perform the major balance operation for table, all sub classes should override this method. + *

+ * Will be invoked by {@link #balanceCluster(Map)}. If + * {@link HConstants#HBASE_MASTER_LOADBALANCE_BYTABLE} is enabled, we will call this method + * multiple times, one table a time, where we will only pass in the regions for a single table + * each time. If not, we will pass in all the regions at once, and the {@code tableName} will be + * {@link HConstants#ENSEMBLE_TABLE_NAME}. + * @param tableName the table to be balanced + * @param loadOfOneTable region load of servers for the specific one table + * @return List of plans + */ + protected abstract List balanceTable(TableName tableName, + Map> loadOfOneTable); + /** + * Called before actually executing balanceCluster. The sub classes could override this method to + * do some initialization work. + */ + protected void + preBalanceCluster(Map>> loadOfAllTable) { + } + + /** + * Perform the major balance operation for cluster, will invoke + * {@link #balanceTable(TableName, Map)} to do actual balance. + *

+ * THIs method is marked as final which means you should not override this method. See the javadoc + * for {@link #balanceTable(TableName, Map)} for more details. + * @param loadOfAllTable region load of servers for all table + * @return a list of regions to be moved, including source and destination, or null if cluster is + * already balanced + * @see #balanceTable(TableName, Map) + */ @Override - public List - balanceCluster(Map>> loadOfAllTable) { + public final synchronized List + balanceCluster(Map>> loadOfAllTable) { + preBalanceCluster(loadOfAllTable); if (isByTable) { List result = new ArrayList<>(); loadOfAllTable.forEach((tableName, loadOfOneTable) -> { diff --git a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java index a8b161a98a1..57fae119363 100644 --- a/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java +++ b/hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SimpleLoadBalancer.java @@ -122,6 +122,13 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { avgLoadOverall = sum / serverLoadList.size(); } + @Override + protected void + preBalanceCluster(Map>> loadOfAllTable) { + // We need clusterLoad of all regions on every server to achieve overall balanced + setClusterLoad(loadOfAllTable); + } + @Override public void onConfigurationChange(Configuration conf) { float originSlop = slop; @@ -247,7 +254,7 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { * or null if cluster is already balanced */ @Override - public List balanceTable(TableName tableName, + protected List balanceTable(TableName tableName, Map> loadOfOneTable) { long startTime = System.currentTimeMillis(); @@ -466,7 +473,7 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { * max. Together with other regions left to be assigned, we distribute all regionToMove, to the RS * that have less regions in whole cluster scope. */ - public void balanceOverall(List regionsToReturn, + private void balanceOverall(List regionsToReturn, Map serverBalanceInfo, boolean fetchFromTail, MinMaxPriorityQueue regionsToMove, int max, int min) { // Step 1. @@ -613,15 +620,4 @@ public class SimpleLoadBalancer extends BaseLoadBalancer { rp.setDestination(sn); regionsToReturn.add(rp); } - - /** - * Override to invoke {@link #setClusterLoad} before balance, We need clusterLoad of all regions - * on every server to achieve overall balanced - */ - @Override - public synchronized List - balanceCluster(Map>> loadOfAllTable) { - setClusterLoad(loadOfAllTable); - return super.balanceCluster(loadOfAllTable); - } } diff --git a/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java b/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java index 2e33cf19a53..a6ae2acf88a 100644 --- a/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java +++ b/hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestBaseLoadBalancer.java @@ -112,14 +112,9 @@ public class TestBaseLoadBalancer extends BalancerTestBase { } public static class MockBalancer extends BaseLoadBalancer { - @Override - public List - balanceCluster(Map>> loadOfAllTable) { - return null; - } @Override - public List balanceTable(TableName tableName, + protected List balanceTable(TableName tableName, Map> loadOfOneTable) { return null; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java index aa3642affe6..17b2863bdb4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/favored/FavoredNodeLoadBalancer.java @@ -94,7 +94,7 @@ public class FavoredNodeLoadBalancer extends BaseLoadBalancer implements Favored } @Override - public List balanceTable(TableName tableName, + protected List balanceTable(TableName tableName, Map> loadOfOneTable) { // TODO. Look at is whether Stochastic loadbalancer can be integrated with this List plans = new ArrayList<>(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java index 4cb873e0a4e..742863def01 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/FavoredStochasticBalancer.java @@ -665,7 +665,7 @@ public class FavoredStochasticBalancer extends StochasticLoadBalancer implements * implementation. For the misplaced regions, we assign a bogus server to it and AM takes care. */ @Override - public List balanceTable(TableName tableName, + protected List balanceTable(TableName tableName, Map> loadOfOneTable) { if (this.services != null) { List regionPlans = Lists.newArrayList(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/MaintenanceLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/MaintenanceLoadBalancer.java index d009e3b326b..11a21029735 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/MaintenanceLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/MaintenanceLoadBalancer.java @@ -66,12 +66,6 @@ public class MaintenanceLoadBalancer extends Configured implements LoadBalancer return Collections.emptyList(); } - @Override - public List balanceTable(TableName tableName, - Map> loadOfOneTable) { - return Collections.emptyList(); - } - private Map> assign(Collection regions, List servers) { // should only have 1 region server in maintenance mode diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java index 0ffdc43654a..9a7823daa50 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java @@ -405,7 +405,7 @@ public class StochasticLoadBalancer extends BaseLoadBalancer { * should always approach the optimal state given enough steps. */ @Override - public synchronized List balanceTable(TableName tableName, Map balanceTable(TableName tableName, Map> loadOfOneTable) { // On clusters with lots of HFileLinks or lots of reference files, // instantiating the storefile infos can be quite expensive. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java index bedc77685bd..fe82e2579be 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java @@ -123,8 +123,7 @@ public class RSGroupBasedLoadBalancer implements LoadBalancer { } /** - * Override to balance by RSGroup - * not invoke {@link #balanceTable(TableName, Map)} + * Balance by RSGroup. */ @Override public List balanceCluster( @@ -449,40 +448,6 @@ public class RSGroupBasedLoadBalancer implements LoadBalancer { internalBalancer.updateBalancerStatus(status); } - /** - * can achieve table balanced rather than overall balanced - */ - @Override - public List balanceTable(TableName tableName, - Map> loadOfOneTable) { - if (!isOnline()) { - LOG.error(RSGroupInfoManager.class.getSimpleName() - + " is not online, unable to perform balanceTable"); - return null; - } - Map>> loadOfThisTable = new HashMap<>(); - loadOfThisTable.put(tableName, loadOfOneTable); - Pair>>, List> - correctedStateAndRegionPlans; - // Calculate correct assignments and a list of RegionPlan for mis-placed regions - try { - correctedStateAndRegionPlans = correctAssignments(loadOfThisTable); - } catch (IOException e) { - LOG.error("get correct assignments and mis-placed regions error ", e); - return null; - } - Map>> correctedLoadOfThisTable = - correctedStateAndRegionPlans.getFirst(); - List regionPlans = correctedStateAndRegionPlans.getSecond(); - List tablePlans = - this.internalBalancer.balanceTable(tableName, correctedLoadOfThisTable.get(tableName)); - - if (tablePlans != null) { - regionPlans.addAll(tablePlans); - } - return regionPlans; - } - private List getFallBackCandidates(List servers) { List serverNames = null; try { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java index 45b34a1d491..38e19e2a1bc 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java @@ -83,7 +83,7 @@ public class LoadBalancerPerformanceEvaluation extends AbstractHBaseTool { private List servers; private List regions; private Map regionServerMap; - private Map> serverRegionMap; + private Map>> tableServerRegionMap; // Non-default configurations. private void setupConf() { @@ -92,6 +92,7 @@ public class LoadBalancerPerformanceEvaluation extends AbstractHBaseTool { } private void generateRegionsAndServers() { + TableName tableName = TableName.valueOf("LoadBalancerPerfTable"); // regions regions = new ArrayList<>(numRegions); regionServerMap = new HashMap<>(numRegions); @@ -101,7 +102,6 @@ public class LoadBalancerPerformanceEvaluation extends AbstractHBaseTool { Bytes.putInt(start, 0, i); Bytes.putInt(end, 0, i + 1); - TableName tableName = TableName.valueOf("LoadBalancerPerfTable"); RegionInfo hri = RegionInfoBuilder.newBuilder(tableName) .setStartKey(start) .setEndKey(end) @@ -114,12 +114,13 @@ public class LoadBalancerPerformanceEvaluation extends AbstractHBaseTool { // servers servers = new ArrayList<>(numServers); - serverRegionMap = new HashMap<>(numServers); + Map> serverRegionMap = new HashMap<>(numServers); for (int i = 0; i < numServers; ++i) { ServerName sn = ServerName.valueOf("srv" + i, HConstants.DEFAULT_REGIONSERVER_PORT, i); servers.add(sn); serverRegionMap.put(sn, i == 0 ? regions : Collections.emptyList()); } + tableServerRegionMap = Collections.singletonMap(tableName, serverRegionMap); } @Override @@ -174,7 +175,7 @@ public class LoadBalancerPerformanceEvaluation extends AbstractHBaseTool { LOG.info("Calling " + methodName); watch.reset().start(); - loadBalancer.balanceTable(HConstants.ENSEMBLE_TABLE_NAME, serverRegionMap); + loadBalancer.balanceCluster(tableServerRegionMap); System.out.print(formatResults(methodName, watch.elapsed(TimeUnit.MILLISECONDS))); return EXIT_SUCCESS;