Uh oh!
There was an error while loading. Please reload this page.
HBASE-26969:Eliminate MOB renames when SFT is enabled - #4418
Merged
Conversation
Apache-HBase
commented
May 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 10, 2022
💔 -1 overall
This message was automatically generated. |
wchevreuil
left a comment
Contributor
There was a problem hiding this comment.
Have gone through a first review round, got a few nits, plus some clarification questions on some of the changes.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| if (this.rsMobFileCleanerChore != null) { | ||
| choreService.scheduleChore(rsMobFileCleanerChore); |
Contributor
There was a problem hiding this comment.
So now we will always have this running at RSes, plus the old MobFileCleanerChore running on Master?
ContributorAuthor
There was a problem hiding this comment.
The new MobFileCleanerChore is not the same as the old one. The main cleanup is done by the RSMobFileCleanerChore running on each RS, cleaning up mob files created by regions hosted on that RS. This should cover most of the usecases.
But I still had to run a restricted version of the old MobFileCleanerChore to clean up after mobs made by now-archived regions, because to make sure those are not referenced anymore every hfile have to be checked.
Contributor
There was a problem hiding this comment.
But I still had to run a restricted version of the old MobFileCleanerChore to clean up after mobs made by now-archived regions, because to make sure those are not referenced anymore every hfile have to be checked.
So, MobFileCleanerChore seems to be checking all existing hfiles. Wouldn't that supersede the RSMobFileCleanerChore work? And what if both run at the same time, could both try to archive a same subset of files?
ContributorAuthor
There was a problem hiding this comment.
Let me clarify the differences between the 2 cleaners.
RSMobFileCleanerChore:
- runs on RS to have access to currently written files and active storefile list
- can only archive mob files created by regions hosted on the current RS
- only reads hfiles belonging to regions hosted by the current RS when looking for references
This allow it to do the majority of the cleanup necessary as efficiently as possible
MobFileCleanerChore:
- runs on Master
- can only archive mob files created by archived regions (regions no longer existing in the /data folder). Thanks to this these mob files can no longer be "currently written" so we do not need the data only available on the RS
- reads every single hfile in /data with a mob enabled CF. This is necessary, because this is the only way if any of these mobs has active references
So yes, there is an overlap, the same hfiles could be read by both cleansers but the a mob file could be archived with either one of them. The cleaner on the master is wasteful and not especially elegant, but thanks to the lack of centralized mob tracking we do not have a better way of collecting the references.
Contributor
There was a problem hiding this comment.
can only archive mob files created by archived regions (regions no longer existing in the /data folder). Thanks to this these mob files can no longer be "currently written" so we do not need the data only available on the RS
Got it.
The cleaner on the master is wasteful and not especially elegant, but thanks to the lack of centralized mob tracking we do not have a better way of collecting the references.
Can we trust META here to list SPLIT/MERGE parents? We could then iterate through these list and check for mob files for these regions. Just brainstorming here, ain't sure if the region dir is left lingering in the FS when SPLIT/MERGE parents are cleared from META (in which case, would make such alternative unfeasible).
Apache-HBase
commented
May 13, 2022
💔 -1 overall
This message was automatically generated. |
| } | ||
| //collecting files, MOB included currently being written | ||
| regionMobs.addAll( | ||
| store.getStoreFilesBeingWritten().stream().map(path -> path.getName()) |
Contributor
There was a problem hiding this comment.
I guess this here is because we need to add all currently being written files because we have no means to know if those have mob refs or not.
ContributorAuthor
There was a problem hiding this comment.
It is here because mob compaction/flush runs as part of non-mob store compaction/flush that created it. And because of how these work the most convenient way to keep track of compacting/flushing mob files was to add them to the list of compacting/flushing non-mob files. So this list has everything. But only part of the list is actually interesting for us at any given time.
Comment on lines
+170
to
+173
| LOG.debug("Found: {} active mob refs for table={}", | ||
| referencedMOBs.values().stream().map(inner -> inner.values()) | ||
| .flatMap(lists -> lists.stream()).mapToInt(lists -> lists.size()).sum(), | ||
| htd.getTableName().getNameAsString()); |
Contributor
There was a problem hiding this comment.
nit: check if debug is enabled, otherwise we'll always be computing the debug param.
ContributorAuthor
There was a problem hiding this comment.
That is a very nice catch, thanks!
Comment on lines
+174
to
+180
| referencedMOBs.values().stream().forEach(innerMap -> innerMap.values().stream() | ||
| .forEach(mobFileList -> mobFileList.stream().forEach(LOG::trace))); | ||
Apache-HBase
commented
May 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Jun 8, 2022
Contributor
Have been going back and forth with @BukrosSzabolcs on this, discussing what would be alternatives for a more efficient solution. Unfortunately, our conclusion is that unless we can implement some sort of "global" registry of referenced mob files, there's not much left we can do. Such refactoring would be more suitable under a whole MOB redesign. So for now, I think we are good to go with this PR, which main objective is to avoid usage of temp dirs/renames when FILE SFT is enabled, making MOB behaviour aligned with the conventional regions behaviour. Regarding the latest UT failures, these should actually be addressed by HBASE-27017 commit. I'm good to approve this once we rebase it and get a green UT run. Thanks for being super patient, @BukrosSzabolcs ! |
Get rid of renames in the MOB compaction and flush. Also refactor the MOB cleaner to be compatible with FileBased StoreFileTracker
Apache-HBase
commented
Jun 9, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 9, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 9, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 13, 2022
💔 -1 overall
This message was automatically generated. |
BukrosSzabolcs
commented
Jun 13, 2022
ContributorAuthor
recheck |
Apache-HBase
commented
Jun 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 14, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 14, 2022
💔 -1 overall
This message was automatically generated. |
BukrosSzabolcs
commented
Jun 17, 2022
ContributorAuthor
recheck |
Apache-HBase
commented
Jun 17, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 17, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 17, 2022
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Jun 20, 2022
wchevreuil
commented
Jun 20, 2022
Contributor
The reported test failures seems unrelated. |
wchevreuil
marked this pull request as ready for review
June 21, 2022 09:17
BukrosSzabolcs added a commit
to BukrosSzabolcs/hbase
that referenced
this pull request
Aug 17, 2022
HBASE-25970:MOB data loss - incorrect concatenation of MOB_FILE_REFS Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
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
Sign up for freeto 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.
Get rid of renames in the MOB compaction and flush. Also refactor the
MOB cleaner to be compatible with FileBased StoreFileTracker