HBASE-23932 Minor improvements to Region Normalizer (apache#1258)

- consolidate checks made by master on behalf of balancer and
   normalizer: deciding if the master is in a healthy state for
   running any actions at all (skipRegionManagementAction). Normalizer
   now does as balancer did previously.
 - both balancer and normalizer make one final check on above
   conditions between calculating an action plan and executing the
   plan. should make the process more responsive to shutdown
   requests.
 - change normalizer to only consider acting on a region when it is in
   the OPEN state. previously we would normalizer attempt to merge a
   region that was already in a MERGING_NEW,MERGING,MERGED state.
 - fix some typos in variable names.

Signed-off-by: Josh Elser <elserj@apache.org>
Signed-off-by: binlijin <binlijin@gmail.com>

Author: Nick Dimiduk
Reason: Improvement
Ref: CDPD-15964

Change-Id: Ia6dc416f41273e1d46cf298c500fb57aecb9288a
(cherry picked from commit 5df8b0d)

Author: ndimiduk

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -78,6 +78,7 @@
 import org.apache.hadoop.hbase.TableNotDisabledException;
 import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.hadoop.hbase.UnknownRegionException;
+import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
 import org.apache.hadoop.hbase.client.MasterSwitchType;
 import org.apache.hadoop.hbase.client.RegionInfo;
@@ -220,6 +221,7 @@
 import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet;
 import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
 import org.apache.hbase.thirdparty.com.google.common.collect.Maps;
+import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils;
 
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState;
@@ -364,7 +366,7 @@ public void run() {
   volatile boolean serviceStarted = false;
 
   // Maximum time we should run balancer for
-  private final int maxBlancingTime;
+  private final int maxBalancingTime;
   // Maximum percent of regions in transition when balancing
   private final double maxRitPercent;
 
@@ -524,7 +526,7 @@ public HMaster(final Configuration conf)
       // preload table descriptor at startup
       this.preLoadTableDescriptors = conf.getBoolean("hbase.master.preload.tabledescriptors", true);
 
-      this.maxBlancingTime = getMaxBalancingTime();
+      this.maxBalancingTime = getMaxBalancingTime();
       this.maxRitPercent = conf.getDouble(HConstants.HBASE_MASTER_BALANCER_MAX_RIT_PERCENT,
           HConstants.DEFAULT_HBASE_MASTER_BALANCER_MAX_RIT_PERCENT);
 
@@ -1638,21 +1640,36 @@ public boolean balance() throws IOException {
     return balance(false);
   }
 
-  public boolean balance(boolean force) throws IOException {
-    // if master not initialized, don't run balancer.
+  /**
+   * Checks master state before initiating action over region topology.
+   * @param action the name of the action under consideration, for logging.
+   * @return {@code true} when the caller should exit early, {@code false} otherwise.
+   */
+  private boolean skipRegionManagementAction(final String action) {
     if (!isInitialized()) {
-      LOG.debug("Master has not been initialized, don't run balancer.");
-      return false;
+      LOG.debug("Master has not been initialized, don't run {}.", action);
+      return true;
+    }
+    if (this.getServerManager().isClusterShutdown()) {
+      LOG.info("Cluster is shutting down, don't run {}.", action);
+      return true;
     }
-
     if (isInMaintenanceMode()) {
-      LOG.info("Master is in maintenanceMode mode, don't run balancer.");
+      LOG.info("Master is in maintenance mode, don't run {}.", action);
+      return true;
+    }
+    return false;
+  }
+
+  public boolean balance(boolean force) throws IOException {
+    if (loadBalancerTracker == null || !loadBalancerTracker.isBalancerOn()) {
+      return false;
+    }
+    if (skipRegionManagementAction("balancer")) {
       return false;
     }
 
     synchronized (this.balancer) {
-      // If balance not true, don't run balancer.
-      if (!this.loadBalancerTracker.isBalancerOn()) return false;
         // Only allow one balance run at at time.
       if (this.assignmentManager.hasRegionsInTransition()) {
         List<RegionStateNode> regionsInTransition = assignmentManager.getRegionsInTransition();
@@ -1701,6 +1718,11 @@ public boolean balance(boolean force) throws IOException {
 
       List<RegionPlan> plans = this.balancer.balanceCluster(assignments);
 
+      if (skipRegionManagementAction("balancer")) {
+        // make one last check that the cluster isn't shutting down before proceeding.
+        return false;
+      }
+
       List<RegionPlan> sucRPs = executeRegionPlansWithThrottling(plans);
 
       if (this.cpHost != null) {
@@ -1721,10 +1743,10 @@ public List<RegionPlan> executeRegionPlansWithThrottling(List<RegionPlan> plans)
     List<RegionPlan> sucRPs = new ArrayList<>();
     int maxRegionsInTransition = getMaxRegionsInTransition();
     long balanceStartTime = System.currentTimeMillis();
-    long cutoffTime = balanceStartTime + this.maxBlancingTime;
+    long cutoffTime = balanceStartTime + this.maxBalancingTime;
     int rpCount = 0;  // number of RegionPlans balanced so far
     if (plans != null && !plans.isEmpty()) {
-      int balanceInterval = this.maxBlancingTime / plans.size();
+      int balanceInterval = this.maxBalancingTime / plans.size();
       LOG.info("Balancer plans size is " + plans.size() + ", the balance interval is "
           + balanceInterval + " ms, and the max number regions in transition is "
           + maxRegionsInTransition);
@@ -1742,18 +1764,18 @@ public List<RegionPlan> executeRegionPlansWithThrottling(List<RegionPlan> plans)
         //rpCount records balance plans processed, does not care if a plan succeeds
         rpCount++;
 
-        if (this.maxBlancingTime > 0) {
+        if (this.maxBalancingTime > 0) {
           balanceThrottling(balanceStartTime + rpCount * balanceInterval, maxRegionsInTransition,
             cutoffTime);
         }
 
         // if performing next balance exceeds cutoff time, exit the loop
-        if (this.maxBlancingTime > 0 && rpCount < plans.size()
+        if (this.maxBalancingTime > 0 && rpCount < plans.size()
           && System.currentTimeMillis() > cutoffTime) {
           // TODO: After balance, there should not be a cutoff time (keeping it as
           // a security net for now)
           LOG.debug("No more balancing till next balance run; maxBalanceTime="
-              + this.maxBlancingTime);
+              + this.maxBalancingTime);
           break;
         }
       }
@@ -1775,47 +1797,47 @@ public RegionNormalizer getRegionNormalizer() {
    *    is globally disabled)
    */
   public boolean normalizeRegions() throws IOException {
-    if (!isInitialized()) {
-      LOG.debug("Master has not been initialized, don't run region normalizer.");
-      return false;
-    }
-    if (this.getServerManager().isClusterShutdown()) {
-      LOG.info("Cluster is shutting down, don't run region normalizer.");
+    if (regionNormalizerTracker == null || !regionNormalizerTracker.isNormalizerOn()) {
+      LOG.debug("Region normalization is disabled, don't run region normalizer.");
       return false;
     }
-    if (isInMaintenanceMode()) {
-      LOG.info("Master is in maintenance mode, don't run region normalizer.");
+    if (skipRegionManagementAction("region normalizer")) {
       return false;
     }
-    if (!this.regionNormalizerTracker.isNormalizerOn()) {
-      LOG.debug("Region normalization is disabled, don't run region normalizer.");
+    if (assignmentManager.hasRegionsInTransition()) {
       return false;
     }
 
     synchronized (this.normalizer) {
       // Don't run the normalizer concurrently
+
       List<TableName> allEnabledTables = new ArrayList<>(
         this.tableStateManager.getTablesInStates(TableState.State.ENABLED));
 
       Collections.shuffle(allEnabledTables);
 
       for (TableName table : allEnabledTables) {
-        if (isInMaintenanceMode()) {
-          LOG.debug("Master is in maintenance mode, stop running region normalizer.");
-          return false;
-        }
-
         TableDescriptor tblDesc = getTableDescriptors().get(table);
         if (table.isSystemTable() || (tblDesc != null &&
             !tblDesc.isNormalizationEnabled())) {
           LOG.trace("Skipping normalization for {}, as it's either system"
               + " table or doesn't have auto normalization turned on", table);
           continue;
         }
-        List<NormalizationPlan> plans = this.normalizer.computePlanForTable(table);
-        if (plans != null) {
+
+        // make one last check that the cluster isn't shutting down before proceeding.
+        if (skipRegionManagementAction("region normalizer")) {
+          return false;
+        }
+
+        final List<NormalizationPlan> plans = this.normalizer.computePlanForTable(table);
+        if (CollectionUtils.isEmpty(plans)) {
+          return true;
+        }
+
+        try (final Admin admin = clusterConnection.getAdmin()) {
           for (NormalizationPlan plan : plans) {
-            plan.execute(clusterConnection.getAdmin());
+            plan.execute(admin);
             if (plan.getType() == PlanType.SPLIT) {
               splitPlanCount++;
             } else if (plan.getType() == PlanType.MERGE) {

hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/AbstractRegionNormalizer.java

@@ -1,4 +1,4 @@
-/**
+/*
  *
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
@@ -21,6 +21,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Objects;
 import org.apache.hadoop.hbase.RegionMetrics;
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.Size;
@@ -30,12 +31,12 @@
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.master.MasterRpcServices;
 import org.apache.hadoop.hbase.master.MasterServices;
+import org.apache.hadoop.hbase.master.RegionState;
+import org.apache.hadoop.hbase.master.assignment.RegionStates;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.protobuf.ServiceException;
-
 import org.apache.hadoop.hbase.shaded.protobuf.RequestConverter;
 
 @InterfaceAudience.Private
@@ -108,12 +109,12 @@ protected boolean isSplitEnabled() {
    */
   protected double getAverageRegionSize(List<RegionInfo> tableRegions) {
     long totalSizeMb = 0;
-    int acutalRegionCnt = 0;
+    int actualRegionCnt = 0;
     for (RegionInfo hri : tableRegions) {
       long regionSize = getRegionSize(hri);
       // don't consider regions that are in bytes for averaging the size.
       if (regionSize > 0) {
-        acutalRegionCnt++;
+        actualRegionCnt++;
         totalSizeMb += regionSize;
       }
     }
@@ -140,73 +141,92 @@ protected double getAverageRegionSize(List<RegionInfo> tableRegions) {
     } else if (targetRegionCount > 0) {
       avgRegionSize = totalSizeMb / (double) targetRegionCount;
     } else {
-      avgRegionSize = acutalRegionCnt == 0 ? 0 : totalSizeMb / (double) acutalRegionCnt;
+      avgRegionSize = actualRegionCnt == 0 ? 0 : totalSizeMb / (double) actualRegionCnt;
     }
 
     LOG.debug("Table {}, total aggregated regions size: {} and average region size {}", table,
       totalSizeMb, avgRegionSize);
     return avgRegionSize;
   }
 
+  /**
+   * Determine if a region in {@link RegionState} should be considered for a merge operation.
+   */
+  private static boolean skipForMerge(final RegionState state) {
+    return state == null || !Objects.equals(state.getState(), RegionState.State.OPEN);
+  }
+
   /**
    * Computes the merge plans that should be executed for this table to converge average region
    * towards target average or target region count
    * @param table table to normalize
    * @return list of merge normalization plans
    */
   protected List<NormalizationPlan> getMergeNormalizationPlan(TableName table) {
-    List<NormalizationPlan> plans = new ArrayList<>();
-    List<RegionInfo> tableRegions =
-        masterServices.getAssignmentManager().getRegionStates().getRegionsOfTable(table);
-    double avgRegionSize = getAverageRegionSize(tableRegions);
-    LOG.debug("Table {}, average region size: {}.\n Computing normalization plan for table: {}, "
+    final RegionStates regionStates = masterServices.getAssignmentManager().getRegionStates();
+    final List<RegionInfo> tableRegions = regionStates.getRegionsOfTable(table);
+    final double avgRegionSize = getAverageRegionSize(tableRegions);
+    LOG.debug("Table {}, average region size: {}. Computing normalization plan for table: {}, "
         + "number of regions: {}",
       table, avgRegionSize, table, tableRegions.size());
 
-    int candidateIdx = 0;
-    while (candidateIdx < tableRegions.size() - 1) {
-      RegionInfo hri = tableRegions.get(candidateIdx);
-      long regionSize = getRegionSize(hri);
-      RegionInfo hri2 = tableRegions.get(candidateIdx + 1);
-      long regionSize2 = getRegionSize(hri2);
+    final List<NormalizationPlan> plans = new ArrayList<>();
+    for (int candidateIdx = 0; candidateIdx < tableRegions.size() - 1; candidateIdx++) {
+      final RegionInfo hri = tableRegions.get(candidateIdx);
+      final RegionInfo hri2 = tableRegions.get(candidateIdx + 1);
+      if (skipForMerge(regionStates.getRegionState(hri))) {
+        continue;
+      }
+      if (skipForMerge(regionStates.getRegionState(hri2))) {
+        continue;
+      }
+      final long regionSize = getRegionSize(hri);
+      final long regionSize2 = getRegionSize(hri2);
+
       if (regionSize >= 0 && regionSize2 >= 0 && regionSize + regionSize2 < avgRegionSize) {
-        // atleast one of the two regions should be older than MIN_REGION_DURATION days
+        // at least one of the two regions should be older than MIN_REGION_DURATION days
         plans.add(new MergeNormalizationPlan(hri, hri2));
         candidateIdx++;
       } else {
         LOG.debug("Skipping region {} of table {} with size {}", hri.getRegionNameAsString(), table,
           regionSize);
       }
-      candidateIdx++;
     }
     return plans;
   }
 
+  /**
+   * Determine if a region in {@link RegionState} should be considered for a split operation.
+   */
+  private static boolean skipForSplit(final RegionState state) {
+    return state == null || !Objects.equals(state.getState(), RegionState.State.OPEN);
+  }
+
   /**
    * Computes the split plans that should be executed for this table to converge average region size
    * towards target average or target region count
    * @param table table to normalize
    * @return list of split normalization plans
    */
   protected List<NormalizationPlan> getSplitNormalizationPlan(TableName table) {
-    List<NormalizationPlan> plans = new ArrayList<>();
-    List<RegionInfo> tableRegions =
-        masterServices.getAssignmentManager().getRegionStates().getRegionsOfTable(table);
-    double avgRegionSize = getAverageRegionSize(tableRegions);
+    final RegionStates regionStates = masterServices.getAssignmentManager().getRegionStates();
+    final List<RegionInfo> tableRegions = regionStates.getRegionsOfTable(table);
+    final double avgRegionSize = getAverageRegionSize(tableRegions);
     LOG.debug("Table {}, average region size: {}", table, avgRegionSize);
 
-    int candidateIdx = 0;
-    while (candidateIdx < tableRegions.size()) {
-      RegionInfo hri = tableRegions.get(candidateIdx);
+    final List<NormalizationPlan> plans = new ArrayList<>();
+    for (final RegionInfo hri : tableRegions) {
+      if (skipForSplit(regionStates.getRegionState(hri))) {
+        continue;
+      }
       long regionSize = getRegionSize(hri);
       // if the region is > 2 times larger than average, we split it, split
       // is more high priority normalization action than merge.
       if (regionSize > 2 * avgRegionSize) {
-        LOG.info("Table {}, large region {} has size {}, more than twice avg size, splitting",
-          table, hri.getRegionNameAsString(), regionSize);
+        LOG.info("Table {}, large region {} has size {}, more than twice avg size {}, splitting",
+          table, hri.getRegionNameAsString(), regionSize, avgRegionSize);
         plans.add(new SplitNormalizationPlan(hri, null));
       }
-      candidateIdx++;
     }
     return plans;
   }