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:
[email protected]
