HBASE-28742 Fixes NPE for CompactionTool when mslab enabled #6109
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
|
💔 -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. |
vineet4008
force-pushed
the
npe-fix-compact-br-2
branch
from
516f124 to
3a3c203
Compare
|
🎊 +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. |
Apache9
reviewed
Jul 23, 2024
| @@ -65,6 +79,7 @@ public void setUp() throws Exception { | |||
| public void tearDown() throws Exception { | |||
| this.testUtil.shutdownMiniCluster(); | |||
| testUtil.cleanupTestDir(); | |||
| ChunkCreator.instance = null; | |||
There was a problem hiding this comment.
Will this cause memory leak? And since we will always reset USEMSLAB_KEY to false, do we really need to do this?
There was a problem hiding this comment.
Setting it to null after shutdown of hbase cluster in test, so that it is not available for next test case in the file. Memory blocks of ChunkCreator should be collected by GC as cluster is shutdown.
| testUtil.startMiniMapReduceCluster(); | ||
| try { | ||
| Configuration config = HBaseConfiguration.create(testUtil.getConfiguration()); | ||
| config.setBoolean(MemStoreLAB.USEMSLAB_KEY, true); |
There was a problem hiding this comment.
Do we still need to do this after parameterized?
There was a problem hiding this comment.
Parameterization is for Server Config. This config is for CompactionTool(Always passing true). Issue happens when mslab is true for Compactiontool. Need to do mslab as false for server, so that ChunkCreator is not created, Otherwise CompactionTool flow was using the Server ChunkCreator instance as test is running in same jvm instance.
There was a problem hiding this comment.
I do not get the point here...
We will run the test method twice with USEMSLAB_KEY as true and false, so we can cover the scenario where we pass USEMSLAB_KEY as true? If we only want it to be true, we should give this test method a separated test class?
There was a problem hiding this comment.
Base test is about functional verification of the feature. For this issue, we can add test cases to a new file. I can revert to test code which I have initially pushed. What is your opinion?
Initial push: New Test was deriving from TestCompactionTool and was verifying MapReduce and Non-MapReduce flow
There was a problem hiding this comment.
If you want to add a new testcase, then just introduce a new test class so we do not need to run the test method in parent class twice? This is my very first question...
There was a problem hiding this comment.
Test code update done. Please review...
vineet4008
force-pushed
the
npe-fix-compact-br-2
branch
from
3a3c203 to
e198e59
Compare
|
🎊 +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. |
Apache9
approved these changes
Jul 27, 2024
Apache9
added
the
backport
Apache9
pushed a commit
that referenced
this pull request
Jul 27, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> (cherry picked from commit 17a68ae)
Apache9
pushed a commit
that referenced
this pull request
Jul 27, 2024
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> (cherry picked from commit 17a68ae)
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.