HBASE-25745 Deprecate/Rename config `hbase.normalizer.min.region.coun… #3139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Ping @ndimiduk |
ndimiduk
requested changes
Apr 29, 2021
| 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); |
There was a problem hiding this comment.
I believe you can register the old key via the deprecation API 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.
There was a problem hiding this comment.
The deprecation API is good, but has some confusion as I said in the jira. When we call conf.addDeprecation(deprecatedConf, newConf), and only set newConf, conf.get(deprecatedConf) always get value even it not set.
There was a problem hiding this comment.
The key point is how to recognize that the user has used the old configuration and log warnings once.
There was a problem hiding this comment.
And I found after conf.addDeprecation(deprecatedConf, newConf), the conf.get(deprecatedConf) can not get value even we configured in conf file. So if the configuration is not modified before upgrade to HBase-2.5, then the
configuration will be use default value. I don't know if I express cleared...
There was a problem hiding this comment.
The example of the use of conf deprecation from block cache that you mention on Jira is, in my opinion, an example that should not be followed.
And I found after conf.addDeprecation(deprecatedConf, newConf), the conf.get(deprecatedConf) can not get value even we configured in conf file. So if the configuration is not modified before upgrade to HBase-2.5, then the configuration will be use default value.
Okay, this is bad.
There was a problem hiding this comment.
I think we need to keep the old config in hbase-defaults.xml and add the new config there as well.
modified hbase-common/src/main/resources/hbase-default.xml
@@ -41,6 +41,8 @@ possible configurations would overwhelm and obscure the important.
<configuration>
<!--Configs you will likely change are listed here at the top of the file.
-->
+ <property><name>foo</name><value>42</value><description>deprecated; see 'bar'</description></property>
+ <property><name>bar</name><value>42</value></property>
<property >
<name>hbase.tmp.dir</name>
<value>${java.io.tmpdir}/hbase-${user.name}</value>
---
| Welcome to JShell -- Version 11.0.10
| For an introduction type: /help intro
jshell> import org.apache.hadoop.conf.Configuration
jshell> import org.apache.hadoop.hbase.HBaseConfiguration
jshell> Configuration c = HBaseConfiguration.create()
c ==> Configuration: core-default.xml, core-site.xml, h ... efault.xml, hbase-site.xml
jshell> c.get("foo")
$4 ==> "42"
jshell> c.get("bar")
$5 ==> "42"
jshell> Configuration.addDeprecation("foo", "bar")
jshell> c.get("foo")
2021-05-03T14:03:00,038 INFO [main] Configuration.deprecation: foo is deprecated. Instead, use bar
$7 ==> "42"
jshell> c.get("bar")
$8 ==> "42"
jshell> c.set("foo", "99")
jshell> c.get("foo")
$10 ==> "99"
jshell> c.get("bar")
$11 ==> "99"
There was a problem hiding this comment.
Your example is special because you call the c.set() method. But we all define the configuration in hbase-site.xml. If your hbase-site.xml define: foo99 and do not call c.set("foo", "99"), the value of c.get("bar") and c.get("foo") is "42". If you call Configuration.addDeprecation("foo", "bar") first, then call Configuration.addDeprecation("foo", "bar"), the value of c.get("bar") and c.get("foo") is 99.
| @@ -370,6 +371,35 @@ public void testHonorsMergeEnabledInTD() { | |||
|
|
|||
| @Test | |||
| public void testHonorsMinimumRegionCount() { | |||
| conf.setInt(MERGE_MIN_REGION_COUNT_KEY, 1); | |||
There was a problem hiding this 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.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Here is my test for Configuration.addDeprecation(). Test 1, create a Configuration object first: public static void main(String[] args) { } ` @deprecated public static void main(String[] args) { } Test 3, hbase-default.xml is empty, and hbase-site.xml has:deprecate.conf99 new.conf42 } ` } |
|
At the beginning, I imitated other deprecated configurations and tried to use Configuration.addDeprecation(), I found that our use of Configuration.addDeprecation() was wrong. The key is that the order of calling is wrong, we should addDeprecation() before init Configuration object. We can see org.apache.hadoop.hdfs.HdfsConfiguration: https://github.com/apache/hadoop/blob/b93e448f9aa66689f1ce5059f6cdce8add130457/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/HdfsConfiguration.java#L34 . |
First or in addition to, I think is up to you. It looks like you've identified an issue in our usage of this Hadoop API, and have worked out a testing strategy -- this is great! I find 10 usages of |
|
Create an issue HBASE-25861 to resolve the problem first. |
…t` to `hbase.normalizer.merge.min.region.count`
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
HBASE-25861 is resolved, and I modified this PR. Please review @ndimiduk |
ndimiduk
approved these changes
May 24, 2021
| if (stringValue == null) { | ||
| stringValue = context.getOrDefault(MIN_REGION_COUNT_KEY, Function.identity(), null); | ||
| if (stringValue != null) { | ||
| LOG.debug("The config key {} in table descriptor is deprecated. Instead please use {}. " |
There was a problem hiding this comment.
huh. maybe TableDescriptor should handle deprecations as well. For another patch.
|
@ZhaoBQ the master patch has conflicts when I backport it to branch-2. I'm guessing there's a patch on master that's missing on branch-2, because branch-2 doesn't have the |
|
Thanks @ndimiduk, let me do the backport. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…t
tohbase.normalizer.merge.min.region.count`