HBASE-25861 Correct the usage of Configuration#addDeprecation #3249
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. |
ndimiduk
requested changes
May 10, 2021
There was a problem hiding this comment.
@ZhaoBQ this change is great!
My comments are entirely around preserving and not growing our public API surface area. You touch several classes that are InterfaceAudience.Public, so care must be taken to not alter their APIs.
One other idea: with javdoc, you can use '{@value ...}' instead of {@link ...} and the value of the constant will be generated into the Javadoc. In this case, printing the rendered string value of the configuration point is preferable to the symbol expression that stores the value. For an example look in org.apache.hadoop.hbase.master.normalizer.package-info.java.
| */ | ||
| @Deprecated | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG) | ||
| public static final String RS_HOSTNAME_KEY = "hbase.regionserver.hostname"; |
There was a problem hiding this comment.
There's no reason to add all these constants to this HConstants class; it's better if we don't expand our public interface if we can avoid it. I think it would be better to use the configuration string values directly in the new HBaseConfiguration#addDeprecatedKeys().
Please do preserve the comments about removal target release.
There was a problem hiding this comment.
I agree w/ the Nick comment.
This file of all the constants is a bit of an anti-pattern... should have gone away long ago.... Was hoping it would shrivel but it hasn't. Don't pour water on it (smile).
| @@ -18,6 +18,12 @@ | |||
|
|
|||
| package org.apache.hadoop.hbase.io; | |||
|
|
|||
| import static org.apache.hadoop.hbase.HConstants.ALLOCATOR_POOL_ENABLED_KEY; | |||
There was a problem hiding this comment.
My above suggestion would mean that these symbols simply don't exist anymore for tests to make use of. I think this is preferable, if test's make use of the "public" api of the product, not the classes. This way, if a configuration key is changed in the class, and that configuration key is used by value in the test, the test would presumably fail.
So in this case, the test will define new constants that contain these configuration key strings.
| @@ -40,8 +40,7 @@ | |||
| public static final String MOB_DIR_NAME = "mobdir"; | |||
| public static final String MOB_REGION_NAME = ".mob"; | |||
| public static final byte[] MOB_REGION_NAME_BYTES = Bytes.toBytes(MOB_REGION_NAME); | |||
| public static final String MOB_CLEANER_PERIOD = "hbase.master.mob.cleaner.period"; | |||
There was a problem hiding this comment.
Unfortunately this class in InterfaceAudience.Public, so these symbols cannot change.
|
Thanks @ndimiduk and @saintstack for review. It's cleaner after using the configuration string values directly in the HBaseConfiguration#addDeprecatedKeys(). |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Yes, this looks much better. It seems the test failure is related though, mind taking a look? If the jenkins test result screen doesn't include enough information for you, you can download all the test logs. |
|
🎊 +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. |
|
The failed UTs can pass in my local env. |
ndimiduk
reviewed
May 13, 2021
There was a problem hiding this comment.
I agree these test failures look unrelated.
I have one more request of you for this change, which is to preserve the comments regarding the deprecations in their new location. The reason is we will eventually file tickets to remove the old configs, and it's important to track when the deprecations were first added. These comments will help future maintainers track the exact version when the old configs can be deleted.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/ByteBuffAllocator.java
Show resolved
Hide resolved
…t configured, the logs will still be printed
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
approved these changes
May 14, 2021
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
May 14, 2021
…#3249) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
May 17, 2021
…#3249) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
ndimiduk
added a commit
that referenced
this pull request
May 18, 2021
Co-authored-by: Baiqiang Zhao <zbq.dean@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
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.
No description provided.