-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28432 Refactor tools which are under test packaging to a new module hbase-diagnostics #6258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Supersedes PR #6249, also see #6249 (comment) Change Summary
Also ran basic commands for sanity of above 5 tools in a standalone local cluster. Newly generated jars' content: CC: @stoty |
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
Show resolved
Hide resolved
6972bdb to
96dd5a6
Compare
|
This PR is ready for review! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...src/main/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java
Show resolved
Hide resolved
hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/security/HBaseKerberosUtils.java
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java
Show resolved
Hide resolved
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java
Show resolved
Hide resolved
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the effort here! This is a really great set of improvements.
This reminds me that I need to go back and read "Working Effectively with Legacy Code" by Michael Feathers.
hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/security/HBaseKerberosUtils.java
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/KeyProviderForTesting.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hbase/master/balancer/LoadBalancerPerformanceEvaluation.java
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/DiagnosticToolsCommonUtils.java
Outdated
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java
Show resolved
Hide resolved
| public static final String OPT_DATA_BLOCK_ENCODING = | ||
| ColumnFamilyDescriptorBuilder.DATA_BLOCK_ENCODING.toLowerCase(Locale.ROOT); | ||
| /** Column family used by the test */ | ||
| public static byte[] DEFAULT_COLUMN_FAMILY = Bytes.toBytes("test_cf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details of the schema used by a particular utility should be isolated to that utility, right? Or is there some intention behind sharing schema across these utilities? I'd rather see these kinds of constants repeated in each utility where they're used.
Or maybe this change is big enough as it is, we can save further cleanup for a separate PR.
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java
Show resolved
Hide resolved
|
I should also say -- we shouldn't be shy about having more maven modules. Let's have a common module for |
As Duo is starting 3.0 preparations, maybe we should leave that for a later ticket. |
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/WALPerformanceEvaluationUtil.java
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/wal/WALPerformanceEvaluation.java
Show resolved
Hide resolved
|
Commit d6d6246 details:
This PR may require multiple rounds of review, thank you for your patience. I will try to keep posting new commits in small bites based on feedbacks I receive here. Should we also create a new module for chaos? I was thinking of doing it as another task of HBASE-28431, if needed? Or else can do i her itself, just that this PR will keep getting larger and larger |
We can decide if we should also target this in current PR. I am okay to update based on this suggested change either here or as another task. |
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/KerberosUtils.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this review a few days ago, but forgot to actually send it, and I was wondering why you haven't responded...
hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/security/HBaseKerberosUtils.java
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/KeyProviderForTesting.java
Outdated
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/LoadTestUtil.java
Show resolved
Hide resolved
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/util/WALPerformanceEvaluationUtil.java
Show resolved
Hide resolved
...diagnostics/src/test/java/org/apache/hadoop/hbase/wal/TestBoundedRegionGroupingStrategy.java
Show resolved
Hide resolved
|
Sure, no problem, I'm just worried that the refactors won't be ready in time for 3.0. |
Hey, I have update the PR as per review comment. Please see if we are good to go. For this PR, built code, started local hbase instance and ran basic sanity for all of the above 6 tools.
Also, let me resume progress for #6184, which is almost ready, so that we can finish these 2 tasks ASAP. |
This comment has been minimized.
This comment has been minimized.
- Updated ITs to have local util for all shares data/confs - Cleaned up other classes in similar manner - Fix test case failure due to log4j - Also added WALPerformanceEvaluation tool which was missed in previous change - Renamed DiagnosticToolsCommonUtils to KerberosUtils - Moved MockRegionServerServices to corresponding main code as needed by WALPerformanceEvaluation - Copied method required by WALPerformanceEvaluation to WALPerformanceEvaluationUtil (draft, need to see can we do any better here) - Added a new class out of TestFSHLogProvider and extracted only test dependent on WALPerformanceEvaluation
- Rename KeyProviderForTesting.java to MockAesKeyProvider.java - Added class level comments for copied / moved code. - Break TestBoundedRegionGroupingStrategy to TestBoundedRegionGroupingStrategyWPETool refactoring tests needing WALPerformanceEvaluation into it
- Moved test dependencies after compile dependencies - Removed unused hbase-server dependency - Added class level comments for copied / moved code.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stoty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should add -diagnostics to the client tarball as well.
(Unless the client tarball is missing some of the dependencies required)
nit: for consistency, IMO we should also add the diagnostics test jar to the assembly, even though we plan to remove all of them in the next step.
Sure let me handle this
I had checked this for other newer modules: we were not adding test jars for any of them, so I think it is okay to not add. As we anyways plan to remove again in follow up work |
ca8bb3d to
01826de
Compare
Fixed. |
stoty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…dule hbase-diagnostics (apache#6258) - Move PerformanceEvaluation, LoadTestTool, HFilePerformanceEvaluation, ScanPerformanceEvaluation, LoadBalancerPerformanceEvaluation, and WALPerformanceEvaluation to a new module: hbase-diagnostics Signed-off-by: Istvan Toth <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 84f4fb3)
…dule hbase-diagnostics (#6258) (#6367) - Move PerformanceEvaluation, LoadTestTool, HFilePerformanceEvaluation, ScanPerformanceEvaluation, LoadBalancerPerformanceEvaluation, and WALPerformanceEvaluation to a new module: hbase-diagnostics Signed-off-by: Istvan Toth <[email protected]> Signed-off-by: Nick Dimiduk <[email protected]> (cherry picked from commit 84f4fb3)
The purpose of this task is to refactor and move certain tools currently located under the test packaging to a new module, named 'hbase-diagnostics'.
The following tools have been initially identified for relocation(will add more as and when identified):
These tools are valuable beyond the scope of testing and should be accessible in the binary distribution of HBase. However, their current location within the test jars adds unnecessary bloat to the assembly and classpath, and potentially introduces CVE-prone JARs into the binary assemblies. We plan to remove all test jars from assembly with HBASE-28433.
This task involves creating the new 'hbase-diagnostics' module, and moving the identified tools into this module. It also includes ensuring that these tools function correctly in their new location and that their relocation does not negatively impact any existing functionality or dependencies.
Also see draft patch without this change for follow up work HBASE-28433 at #6184