HBASE-28742 Fixes NPE for CompactionTool when mslab enabled #6097
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. |
|
@Apache9 @bbeaudreault Please review |
|
So, if memstore lab is enabled, and we actually has a memstore, but there is no problem if we do not initialize the memstore chunk, but there will be an NPE while closing the memstore? Seems really strange... Do we really need memstore in CompactionTool? If not, I think we can just disable memstore lab to fix the problem for now? |
vineet4008
force-pushed
the
npe-fix-compact
branch
from
1020699 to
cf2b0cb
Compare
Yes, memstore lab can be disabled. Have pushed the fix. |
pankaj72981
approved these changes
Jul 19, 2024
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
vineet4008
force-pushed
the
npe-fix-compact
branch
from
cf2b0cb to
7b9c4f4
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Do we have a test for CompactionTool? I'm not very familiar with the code so I'm not sure whether it will use memstore, I guess it will not use, but better has a test to confirm this. Thanks. |
vineet4008
force-pushed
the
npe-fix-compact
branch
from
7b9c4f4 to
6deefef
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Added Test to validate the issue and fix |
Apache9
reviewed
Jul 22, 2024
| * Test the issue of HBASE-28472 | ||
| */ | ||
| @Category({ MediumTests.class, RegionServerTests.class }) | ||
| public class TestCompactionToolNpeFix extends TestCompactionTool { |
There was a problem hiding this comment.
Do we really need to extends TestCompactionTool? Do we need to run the test in the parent class twice?
There was a problem hiding this comment.
Thanks @Apache9 for review.
In TestCompactionTool, when system is started with mslab enabled, then ChunkCreator is created and initialized. So when test case runs CompactionTool, it does not report NPE.
To reproduce the issue, setting mslab to disabled in derived class, and then base test fails with NPE as ChunkCreator is NULL.
There was a problem hiding this comment.
Then we can just change TestCompactionTool Parameterized? What is the purpose for the newly added test method?
There was a problem hiding this comment.
Other test case is added to test the CompactionTool MapReduce flow. Existing test case verifies the Non MapReduce flow.
There was a problem hiding this comment.
Then I think we should first change TestCompactionTool Parameterized, and then introduce a new tests? The current architecture seems really strange...
There was a problem hiding this comment.
Changed TestCompactionTool to Parameterized and added new cases in the same test file.
vineet4008
force-pushed
the
npe-fix-compact
branch
from
6deefef to
d7b397e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
vineet4008
force-pushed
the
npe-fix-compact
branch
from
d7b397e to
9d623ee
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Jul 27, 2024
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 43b1d78)
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.
Fixes NPE issue with initialization of ChunkCreator by disabling the mslab