[GitHub] [hbase] ndimiduk commented on a change in pull request #2917: HBASE-25534 Improve the configuration of Normalizer

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

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -254,41 +255,38 @@ private boolean isMasterSwitchEnabled(final MasterSwitchType masterSwitchType) {
     return masterServices.isSplitOrMergeEnabled(masterSwitchType);
   }
 
-  private boolean proceedWithSplitPlanning() {
-    return isSplitEnabled() && isMasterSwitchEnabled(MasterSwitchType.SPLIT);
+  private boolean proceedWithSplitPlanning(TableDescriptor tableDescriptor) {
+    String value = tableDescriptor.getValue(SPLIT_ENABLED_KEY);
+    return  (value == null ? isSplitEnabled() : Boolean.parseBoolean(value)) &&
+      isMasterSwitchEnabled(MasterSwitchType.SPLIT);
   }
 
-  private boolean proceedWithMergePlanning() {
-    return isMergeEnabled() && isMasterSwitchEnabled(MasterSwitchType.MERGE);
+  private boolean proceedWithMergePlanning(TableDescriptor tableDescriptor) {
+    String value = tableDescriptor.getValue(MERGE_ENABLED_KEY);
+    return (value == null ? isMergeEnabled() : Boolean.parseBoolean(value)) &&
+      isMasterSwitchEnabled(MasterSwitchType.MERGE);
   }
 
   /**
    * @param tableRegions regions of table to normalize
+   * @param tableDescriptor the TableDescriptor
    * @return average region size depending on
    * @see org.apache.hadoop.hbase.client.TableDescriptor#getNormalizerTargetRegionCount()
    * Also make sure tableRegions contains regions of the same table
    */
-  private double getAverageRegionSizeMb(final List<RegionInfo> tableRegions) {
+  private double getAverageRegionSizeMb(final List<RegionInfo> tableRegions,
+    final TableDescriptor tableDescriptor) {
     if (isEmpty(tableRegions)) {
       throw new IllegalStateException(
         "Cannot calculate average size of a table without any regions.");
     }
-    TableName table = tableRegions.get(0).getTable();
-    int targetRegionCount = -1;
-    long targetRegionSize = -1;
+    TableName table = tableDescriptor.getTableName();
     double avgRegionSize;
-    try {
-      TableDescriptor tableDescriptor = masterServices.getTableDescriptors().get(table);
-      if (tableDescriptor != null) {
-        targetRegionCount = tableDescriptor.getNormalizerTargetRegionCount();
-        targetRegionSize = tableDescriptor.getNormalizerTargetRegionSize();
-        LOG.debug("Table {} configured with target region count {}, target region size {}", table,
-          targetRegionCount, targetRegionSize);
-      }
-    } catch (IOException e) {
-      LOG.warn("TableDescriptor for {} unavailable, table-level target region count and size"
-        + " configurations cannot be considered.", table, e);
-    }
+    long targetRegionCount = tableDescriptor.getNormalizerTargetRegionCount();

Review comment:
       nit: no need to promote this to a `long`.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
##########
@@ -292,12 +293,28 @@ public void testHonorsSplitEnabled() {
       createRegionSizesMap(regionInfos, 5, 5, 20, 5, 5);
     setupMocksForNormalizer(regionSizes, regionInfos);
     assertThat(
-      normalizer.computePlansForTable(tableName),
+      normalizer.computePlansForTable(tableDescriptor),
       contains(instanceOf(SplitNormalizationPlan.class)));
 
     conf.setBoolean(SPLIT_ENABLED_KEY, false);
     setupMocksForNormalizer(regionSizes, regionInfos);
-    assertThat(normalizer.computePlansForTable(tableName), empty());
+    assertThat(normalizer.computePlansForTable(tableDescriptor), empty());
+  }
+
+  @Test
+  public void testHonorsSplitEnabledInTD() {

Review comment:
       These tests `*EnabledInTD` are good for confirming that the split/merge will not happen when disabled in table descriptor. Please also add coverage of when split/merge is disabled in configuration but enabled in table descriptor. I believe this should be supported, given the new implementations of `proceedWith{Split,Merge}Planning`.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -316,10 +314,10 @@ private double getAverageRegionSizeMb(final List<RegionInfo> tableRegions) {
    */
   private boolean skipForMerge(
     final NormalizerConfiguration normalizerConfiguration,
-    final RegionStates regionStates,
+    final NormalizeContext ctx,

Review comment:
       good.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -592,5 +621,14 @@ public RegionStates getRegionStates() {
     public double getAverageRegionSizeMb() {
       return averageRegionSizeMb;
     }
+
+    public <T> T getOrDefault(String key, Function<String, T> function, T defaultValue) {

Review comment:
       nice.
   
   Maybe this method returns an `Object`/`null` and the caller handles the null-check. I think this approach would be better than using an out-of-range value to indicate the lack of configuration.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -468,12 +467,13 @@ private static boolean isOldEnoughForMerge(
    */
   private boolean isLargeEnoughForMerge(
     final NormalizerConfiguration normalizerConfiguration,
+    final NormalizeContext ctx,
     final RegionInfo regionInfo
   ) {
-    return getRegionSizeMB(regionInfo) >= normalizerConfiguration.getMergeMinRegionSizeMb();
+    return getRegionSizeMB(regionInfo) >= normalizerConfiguration.getMergeMinRegionSizeMb(ctx);
   }
 
-  private static boolean logTraceReason(final BooleanSupplier predicate, final String fmtWhenTrue,
+  private boolean logTraceReason(final BooleanSupplier predicate, final String fmtWhenTrue,

Review comment:
       why make this an instance method?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -468,12 +467,13 @@ private static boolean isOldEnoughForMerge(
    */
   private boolean isLargeEnoughForMerge(

Review comment:
       can be static now as well?




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