HBASE-28432 Refactor tools which are under test packaging to a new mo… #6249
Conversation
This file contains hidden or 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
Contributor
Author
|
very rough change. lot of things to handle:
Will post a summary of changes and things to be take care of later tomorrow |
NihalJain
force-pushed
the
HBASE-28432
branch
from
ee858f5 to
da6b839
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NihalJain
force-pushed
the
HBASE-28432
branch
from
da6b839 to
469be3d
Compare
Contributor
Author
|
Change Summary
TODO
@ndimiduk, @stoty @Apache9 Would you be able to provide some early feedback for this draft PR.? |
NihalJain
force-pushed
the
HBASE-28432
branch
from
469be3d to
167c153
Compare
This comment has been minimized.
This comment has been minimized.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
stoty
reviewed
Sep 17, 2024
Contributor
stoty
left a comment
stoty
left a comment
There was a problem hiding this comment.
Not having to deal with Copy classes would also make the patch much smaller, half the files only differ due to those renames.
| * Remove after tfile is committed and use the tfile version of this class instead. | ||
| * </p> | ||
| */ | ||
| public class RandomDistributionCopy { |
Contributor
There was a problem hiding this comment.
This is a bad name.
Do we need two copies of this class ? Can't we just move this from hbase-compression to hbase-common with the same name ?
Contributor
There was a problem hiding this comment.
The same applies to the other "Copy" classes.
Contributor
Author
There was a problem hiding this comment.
Hey @stoty Thanks for having a look at the PR.
Do we need two copies of this class ? Can't we just move this from hbase-compression to hbase-common with the same name ?
These files are not supposed to be bundled and are here only for draft change. please see previous comment where I have captured a summary of changes: #6249 (comment)
See
Created a copy just to bring out the all the usages of the class to help decide (based on references) where we move this in final patch:
RandomDistribution
KeyProviderForTesting
LoadTestKVGenerator
Also see
Deal with copied classes which have cyclic dependencies and remove all copies and its references
Should we copy these to main of hbase-server or hbase-common?
I have created copy to show case how these files are being used across the project. Just want to discuss
- if it is fine to move these files to proper module, outside hbase-diagnostics to resolve cyclic dependencies of usage
- or is it fine to create a new copy them retaining the test copies where ever they are, which IMO is not clean code.
Contributor
Author
There was a problem hiding this comment.
Can't we just move this from hbase-compression to hbase-common with the same name ?
I am +1 on moving to hbase-common. Will wait some time before updating draft with this change, lets also see what other think about this.
Contributor
There was a problem hiding this comment.
Can you add a separete patch on top of this one which moves the classes to hbase-common ?
You can open a separate PR for that one, so this one still shows what you wanted.
Contributor
There was a problem hiding this comment.
I'd like to see how big the patch is without the copies.
Contributor
Author
There was a problem hiding this comment.
Can you add a separete patch on top of this one which moves the classes to hbase-common ?
Sure @stoty let me post a new PR with suggested change.
| table.close(); | ||
|
|
||
| } | ||
| // TODO: Below test seems to be using FilterAllFilter |
Contributor
There was a problem hiding this comment.
Need to resolve this before commit
Contributor
|
Sorry I did not read your comments @NihalJain . |
Contributor
Author
Hey @stoty no problem at all. I have raised a new PR with suggested changes where I will continue rest of the work, will close this one soon |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
…dule hbase-diagnostics
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