[GitHub] [hbase] ndimiduk commented on a change in pull request #3139: HBASE-25745 Deprecate/Rename config `hbase.normalizer.min.region.coun…

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

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



##########
File path: hbase-common/src/main/resources/hbase-default.xml
##########
@@ -638,12 +638,6 @@ possible configurations would overwhelm and obscure the important.
     <value>true</value>
     <description>Whether to merge a region as part of normalization.</description>
   </property>
-  <property>
-    <name>hbase.normalizer.min.region.count</name>

Review comment:
       What's the reasoning for removing the default value from this file? While it's redundant to use in code, this file serves as a source of documentation ; I believe it is parsed by our site building process to populate a section of the online book.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SimpleRegionNormalizer.java
##########
@@ -101,11 +106,21 @@ public void onConfigurationChange(Configuration conf) {
     setConf(conf);
   }
 
-  private static int parseMinRegionCount(final Configuration conf) {
-    final int parsedValue = conf.getInt(MIN_REGION_COUNT_KEY, DEFAULT_MIN_REGION_COUNT);
+  private static int parseMergeMinRegionCount(final Configuration conf) {
+    String parsedStringValue = conf.get(MERGE_MIN_REGION_COUNT_KEY);

Review comment:
       I _believe_ you can register the old key via the [deprecation API](https://hadoop.apache.org/docs/r2.10.1/api/org/apache/hadoop/conf/Configuration.html#addDeprecation(java.lang.String,%20java.lang.String,%20java.lang.String)) and only ever access the new key from code. I believe this has the benefit of only logging the warning once, instead of every time the implementation happens to do a lookup.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/normalizer/TestSimpleRegionNormalizer.java
##########
@@ -370,6 +371,35 @@ public void testHonorsMergeEnabledInTD() {
 
   @Test
   public void testHonorsMinimumRegionCount() {
+    conf.setInt(MERGE_MIN_REGION_COUNT_KEY, 1);

Review comment:
       Can you instead keep a single implementation in a helper method that allows for the configuration used to be passed in as a parameter? This would be better than copy-pasting the unit test method body.




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