HBASE-24687: Use existing HMaster Connection in MobFileCleanerChore #5509
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
charlesconnell
force-pushed
the
HBASE-24687/mob-file-cleaner-chore-connection
branch
from
17a741d to
059fcc4
Compare
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
reviewed
Nov 7, 2023
There was a problem hiding this comment.
Nice, that was easier than the original PR let on. I think you're going to need to update TestMobFileCleanerChore though. I expect pre-commit hooks will fail with an NPE since master is null when no-arg constructor is used (as in that test and a couple others). We probably should delete the no-arg constructor.
|
Also please fix checkstyle and spotless |
bbeaudreault
reviewed
Nov 7, 2023
hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobFileCleanerChore.java
Show resolved
Hide resolved
|
💔 -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. |
|
Looking for a solution to cleanly support the various uses of MobFileCleanerChore in test code, I gave it a constructor that takes an Admin directly. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 8, 2023
| } | ||
|
|
||
| public MobFileCleanerChore() { | ||
| public MobFileCleanerChore(Admin admin) { |
There was a problem hiding this comment.
Where do we use this method? I think a chore should only be executed at master side?
There was a problem hiding this comment.
Typically yes, but this class is also used by MobStressToolRunner and IntegrationTestMobCompaction, which are both effectively HBase clients.
There was a problem hiding this comment.
I guess these classes just want to run the cleanup method, instead of running a background task? Let me check the code...
There was a problem hiding this comment.
OK, checked the code, they both only call the cleanupObsoleteMobFiles method. I think we could introduce something like MobCleanupUtil, and put the cleanupObsoleteMobFiles and archiveMobFiles in it, where we need to change them to static methods, and then call them from the two classes, and also here, in MobFileCleanerChore. In this way we do not need to maintain this constructor then.
|
🎊 +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. |
|
I'm not seeing any evidence that my changes have caused these build failures |
|
Yea, the last failure is unrelated. The other 2 look weird. I restarted the build. |
|
🎊 +1 overall
This message was automatically generated. |
|
@charlesconnell please fix the checkstyle warnings |
|
💔 -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. |
Apache9
reviewed
Nov 13, 2023
| HTU.startMiniCluster(); | ||
| admin = HTU.getAdmin(); | ||
| chore = new MobFileCleanerChore(); | ||
| chore = new MobFileCleanerChore(HTU.getHBaseCluster().getMaster()); |
There was a problem hiding this comment.
Do we still use this chore anywhere in this test?
There was a problem hiding this comment.
Good point, we don't. I've removed this field and renamed the class TestMobFileCleanupUtil.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Nov 13, 2023
There was a problem hiding this comment.
LGTM. The test failure is unrelated.
Anything else on your end @Apache9?
Apache9
approved these changes
Nov 13, 2023
bbeaudreault
deleted the
HBASE-24687/mob-file-cleaner-chore-connection
branch
bbeaudreault
pushed a commit
that referenced
this pull request
Nov 13, 2023
…5509) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
bbeaudreault
pushed a commit
that referenced
this pull request
Nov 13, 2023
…5509) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
bbeaudreault
pushed a commit
that referenced
this pull request
Nov 13, 2023
…5509) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Nov 13, 2023
…FileCleanerChore (apache#5509) Signed-off-by: Bryan Beaudreault <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This connection does not traverse netty and networking stuff, so is lighter weight and avoids confusing Netty with connections whose client side and server side are in the same EventLoopGroup. While the Netty thing is a larger issue, this small fix should be enough to prevent HMaster lockups.