HBASE-25759 The master services field in LocalityBasedCostFunction is…

… never used (#3144) Signed-off-by: Yulin Niu <[email protected]>
apache · Apr 12, 2021 · de012d7 · de012d7
1 parent f9e928e
commit de012d7
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 49 deletions.
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
@@ -55,7 +55,6 @@
 import org.apache.hadoop.hbase.master.RackManager;
 import org.apache.hadoop.hbase.master.RegionPlan;
 import org.apache.hadoop.hbase.master.balancer.BaseLoadBalancer.Cluster.Action.Type;
-import org.apache.hadoop.hbase.namequeues.NamedQueueRecorder;
 import org.apache.hadoop.hbase.net.Address;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
@@ -92,11 +91,6 @@ public abstract class BaseLoadBalancer implements LoadBalancer {
   protected boolean useRegionFinder;
   protected boolean isByTable = false;
 
-  /**
-   * Use to add balancer decision history to ring-buffer
-   */
-  protected NamedQueueRecorder namedQueueRecorder;
-
   private static class DefaultRackManager extends RackManager {
     @Override
     public String getRack(ServerName server) {

diff --git a/...-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java b/...-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
@@ -158,6 +158,11 @@ public class StochasticLoadBalancer extends BaseLoadBalancer {
   private RegionReplicaHostCostFunction regionReplicaHostCostFunction;
   private RegionReplicaRackCostFunction regionReplicaRackCostFunction;
 
+  /**
+   * Use to add balancer decision history to ring-buffer
+   */
+  NamedQueueRecorder namedQueueRecorder;
+
   /**
    * The constructor that pass a MetricsStochasticBalancer to BaseLoadBalancer to replace its
    * default MetricsBalancer
@@ -184,8 +189,8 @@ public synchronized void setConf(Configuration conf) {
     if (localityCandidateGenerator == null) {
       localityCandidateGenerator = new LocalityBasedCandidateGenerator(services);
     }
-    localityCost = new ServerLocalityCostFunction(conf, services);
-    rackLocalityCost = new RackLocalityCostFunction(conf, services);
+    localityCost = new ServerLocalityCostFunction(conf);
+    rackLocalityCost = new RackLocalityCostFunction(conf);
 
     if (this.candidateGenerators == null) {
       candidateGenerators = Lists.newArrayList();
@@ -307,8 +312,6 @@ public void updateMetricsSize(int size) {
   @Override
   public synchronized void setMasterServices(MasterServices masterServices) {
     super.setMasterServices(masterServices);
-    this.localityCost.setServices(masterServices);
-    this.rackLocalityCost.setServices(masterServices);
     this.localityCandidateGenerator.setServices(masterServices);
   }
 
@@ -1058,17 +1061,11 @@ static abstract class LocalityBasedCostFunction extends CostFunction {
     private double bestLocality; // best case locality across cluster weighted by local data size
     private double locality; // current locality across cluster weighted by local data size
 
-    private MasterServices services;
-
-    LocalityBasedCostFunction(Configuration conf,
-                              MasterServices srv,
-                              LocalityType type,
-                              String localityCostKey,
-                              float defaultLocalityCost) {
+    LocalityBasedCostFunction(Configuration conf, LocalityType type, String localityCostKey,
+      float defaultLocalityCost) {
       super(conf);
       this.type = type;
       this.setMultiplier(conf.getFloat(localityCostKey, defaultLocalityCost));
-      this.services = srv;
       this.locality = 0.0;
       this.bestLocality = 0.0;
     }
@@ -1078,21 +1075,12 @@ static abstract class LocalityBasedCostFunction extends CostFunction {
      */
     abstract int regionIndexToEntityIndex(int region);
 
-    public void setServices(MasterServices srvc) {
-      this.services = srvc;
-    }
-
     @Override
     void init(Cluster cluster) {
       super.init(cluster);
       locality = 0.0;
       bestLocality = 0.0;
 
-      // If no master, no computation will work, so assume 0 cost
-      if (this.services == null) {
-        return;
-      }
-
       for (int region = 0; region < cluster.numRegions; region++) {
         locality += getWeightedLocality(region, regionIndexToEntityIndex(region));
         bestLocality += getWeightedLocality(region, getMostLocalEntityForRegion(region));
@@ -1108,9 +1096,6 @@ void init(Cluster cluster) {
     protected void regionMoved(int region, int oldServer, int newServer) {
       int oldEntity = type == LocalityType.SERVER ? oldServer : cluster.serverIndexToRackIndex[oldServer];
       int newEntity = type == LocalityType.SERVER ? newServer : cluster.serverIndexToRackIndex[newServer];
-      if (this.services == null) {
-        return;
-      }
       double localityDelta = getWeightedLocality(region, newEntity) - getWeightedLocality(region, oldEntity);
       double normalizedDelta = bestLocality == 0 ? 0.0 : localityDelta / bestLocality;
       locality += normalizedDelta;
@@ -1136,14 +1121,8 @@ static class ServerLocalityCostFunction extends LocalityBasedCostFunction {
     private static final String LOCALITY_COST_KEY = "hbase.master.balancer.stochastic.localityCost";
     private static final float DEFAULT_LOCALITY_COST = 25;
 
-    ServerLocalityCostFunction(Configuration conf, MasterServices srv) {
-      super(
-          conf,
-          srv,
-          LocalityType.SERVER,
-          LOCALITY_COST_KEY,
-          DEFAULT_LOCALITY_COST
-      );
+    ServerLocalityCostFunction(Configuration conf) {
+      super(conf, LocalityType.SERVER, LOCALITY_COST_KEY, DEFAULT_LOCALITY_COST);
     }
 
     @Override
@@ -1157,14 +1136,8 @@ static class RackLocalityCostFunction extends LocalityBasedCostFunction {
     private static final String RACK_LOCALITY_COST_KEY = "hbase.master.balancer.stochastic.rackLocalityCost";
     private static final float DEFAULT_RACK_LOCALITY_COST = 15;
 
-    public RackLocalityCostFunction(Configuration conf, MasterServices services) {
-      super(
-          conf,
-          services,
-          LocalityType.RACK,
-          RACK_LOCALITY_COST_KEY,
-          DEFAULT_RACK_LOCALITY_COST
-      );
+    public RackLocalityCostFunction(Configuration conf) {
+      super(conf, LocalityType.RACK, RACK_LOCALITY_COST_KEY, DEFAULT_RACK_LOCALITY_COST);
     }
 
     @Override

diff --git a/...ver/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java b/...ver/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
@@ -34,7 +34,6 @@
 import java.util.Set;
 import java.util.TimeZone;
 import java.util.TreeMap;
-
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.ClusterMetrics;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -54,11 +53,12 @@
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
-import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
+import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils;
+
 @Category({ MasterTests.class, MediumTests.class })
 public class TestStochasticLoadBalancer extends BalancerTestBase {
 
@@ -277,7 +277,7 @@ public void testLocalityCost() throws Exception {
     Configuration conf = HBaseConfiguration.create();
     MockNoopMasterServices master = new MockNoopMasterServices();
     StochasticLoadBalancer.CostFunction
-        costFunction = new ServerLocalityCostFunction(conf, master);
+        costFunction = new ServerLocalityCostFunction(conf);
 
     for (int test = 0; test < clusterRegionLocationMocks.length; test++) {
       int[][] clusterRegionLocations = clusterRegionLocationMocks[test];