Uh oh!
There was an error while loading. Please reload this page.
HBASE-26081 Copy HBTU to hbase-testing-util, rename the HBTU related … - #3478
Conversation
Apache9
commented
Jul 13, 2021
This is a very big change. The classes in the new package, org.apache.hadoop.hbase.testing, are all the classes we want to hide from end users, including HBTU and its parent classes, and HBaseCluster and MiniHBaseCluster. All these classes are changed to IA.LimitedPrivate(Phoenix), as I believe Phoenix still needs test some internal stuff of HBase. The DistributedHBaseCluster is kept as is since it is IA.Private. And then I copied the classes under the new org.apache.hadoop.hbase.testing to org.apache.hadoop.hbase in hbase-testing-module, still keep the IA.Public annotation but mark them all as deprecated, which will be removed in 4.0.0, for keeping compatible. Also modified the HBTU a bit to remove the dependency on junit, by throwing AssertionError directly, as I do not think we should pull in junit as a compile dependency for any libraries... Thanks. |
Apache-HBase
commented
Jul 13, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 14, 2021
Apache-HBase
commented
Jul 15, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 15, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 16, 2021
💔 -1 overall
This message was automatically generated. |
saintstack
commented
Jul 16, 2021
So, a user of these classes who moved to hbase 3.0.0 might have to amend their CLASSPATH but that is all? They might have to add hbase-testing jar to their CLASSPATH? |
saintstack
commented
Jul 16, 2021
Why you thinking the package movement? The HBTU I'd think should be a top-level package rather than a sub-package down in testing? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
saintstack
commented
Jul 16, 2021
You move the fundamentals HBTU and HBC from top-level into sub-package to send downstreamers the message that these classes are off-limits going forward? |
Apache-HBase
commented
Jul 16, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 16, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Jul 17, 2021
The idea is to copy the HBTU related classes to hbase-testing-util and mark them deprecated, so end users could still use them through all the hbase 3.x lifecycle, and also have plenty of time to switch to the new TestingHBaseCluster, which is enough for most end users. And for hbase itself, we move the HBTU to another package, so it will not have conflict with the one in hbase-testing-util, and then, since it is not IA.Public again, we could start to purge the uncessary methods, and also do big refactoring on it. And I think Phoenix still needs to make use of this class, as it needs to test something internal to hbase, so I marked the 'new' HBTU as IA.LimitedPrivate, but anyone, I think for an IA.LimitedPrivate class, we are free to change it in minor releases. You can see some discuss in the parent issue HBASE-13126. Thanks. |
Apache9
commented
Jul 17, 2021
Yes, users need to add hbase-testing-util as a test dependency, but they do not need to change their code in 3.0.0, but in 4.0.0, they have to change their code to make use of the new classes. |
Apache9
commented
Jul 17, 2021
To avoid conflict with the copied ones in hbase-testing-util, we should either move the original one to another package, or rename them. In general, I do not have much intention on which one is better, but for me, I do not have better names for these classes, due to lack of English vocabulary... Do you have any suggestions sir? We could move them back to the top level, but with different names. Thanks. |
Apache-HBase
commented
Jul 17, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 17, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 17, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 17, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 17, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
commented
Jul 17, 2021
Thanks for explanation. Suggest you add description on the JIRA repeating the helpful overview you give here pointing at the parent issue's discussion. On moving the classes to a sub-package though some of these classes are used broadly, given what you are trying to achieve (cleanup, refactor), this is probably the easiest approach. One suggestion I have would be renaming HBaseTestingUtility as TestingUtility and leaving it in the top-level package. The 'HBase' prefix is redundant. Might be working looking at. If it makes more work for you, just ignore. Meantime, let me +1 this change. |
Apache-HBase
commented
Jul 17, 2021
🎊 +1 overall
This message was automatically generated. |
…classes in hbase-server and mark them as IA.LimitedPrivate
Apache9
commented
Jul 18, 2021
Thank you @saintstack . Finally I think I found a way to do renaming instead of moving. This is the release note of this issue. PTAL. Shout if you do not think it is good enough.
|
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
Skimmed. Looks great. +1
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2021
🎊 +1 overall
This message was automatically generated. |
…classes in hbase-server and mark them as IA.LimitedPrivate