Uh oh!
There was an error while loading. Please reload this page.
HBASE-26187 Write straight into the store directory when Splitting an… - #3574
Merged
Conversation
…d Merging Change-Id: I5d0bbd99d27a6b3ecdd0a069dc8741239b32edfa
wchevreuil
commented
Aug 10, 2021
Comment on lines
-587
to
-618
| void cleanupSplitsDir() throws IOException { | ||
| deleteDir(getSplitsDir()); | ||
| } | ||
| /** | ||
| * Clean up any split detritus that may have been left around from previous | ||
| * split attempts. | ||
| * Call this method on initial region deploy. | ||
| * @throws IOException | ||
| */ | ||
| void cleanupAnySplitDetritus() throws IOException { | ||
| Path splitdir = this.getSplitsDir(); | ||
| if (!fs.exists(splitdir)) return; | ||
| // Look at the splitdir. It could have the encoded names of the daughter | ||
| // regions we tried to make. See if the daughter regions actually got made | ||
| // out under the tabledir. If here under splitdir still, then the split did | ||
| // not complete. Try and do cleanup. This code WILL NOT catch the case | ||
| // where we successfully created daughter a but regionserver crashed during | ||
| // the creation of region b. In this case, there'll be an orphan daughter | ||
| // dir in the filesystem. TOOD: Fix. | ||
| FileStatus[] daughters = CommonFSUtils.listStatus(fs, splitdir, new FSUtils.DirFilter(fs)); | ||
| if (daughters != null) { | ||
| for (FileStatus daughter: daughters) { | ||
| Path daughterDir = new Path(getTableDir(), daughter.getPath().getName()); | ||
| if (fs.exists(daughterDir) && !deleteDir(daughterDir)) { | ||
| throw new IOException("Failed delete of " + daughterDir); | ||
| } | ||
| } | ||
| } | ||
| cleanupSplitsDir(); | ||
| LOG.info("Cleaned up old failed split transaction detritus: " + splitdir); | ||
| } |
ContributorAuthor
There was a problem hiding this comment.
Since we won't be using temp dirs anymore, these methods would have no use. Any potential failed split/merge dir would be cleaned by CatalogJanitor, once it finds no related entry in meta for these regions.
Apache-HBase
commented
Aug 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2021
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 11, 2021
| // =========================================================================== | ||
| /** @return {@link Path} to the temp directory used during split operations */ | ||
| /** @return {@link Path} to the table directory for split daughters */ | ||
| Path getSplitsDir() { |
ContributorAuthor
There was a problem hiding this comment.
Not really, let me remove it.
| throw new IOException("Failed deletion of " + splitdir + " before creating them again."); | ||
| } | ||
| } | ||
| // splitDir doesn't exists now. No need to do an exists() call for it. |
Contributor
There was a problem hiding this comment.
This comment is incorrect now?
| } | ||
| @Test | ||
| public void testSplitStoreDir() throws Exception { |
Contributor
There was a problem hiding this comment.
Better place the below methods to a different test class? And is it possible to only start mini cluster once? Now every test method needs to start and then stop the mini cluster, which is time consuming...
ContributorAuthor
There was a problem hiding this comment.
Ok, let me move these to a separate class and do the mini cluster init only once.
| // splitDir doesn't exists now. No need to do an exists() call for it. | ||
| if (!createDir(splitdir)) { | ||
| throw new IOException("Failed create of " + splitdir); | ||
| if (!fs.exists(splitdir)) { |
Contributor
There was a problem hiding this comment.
So the split directory is now the table directory, why we still need to check existence here? And IMO here we should not use 'splitDir' and 'mergeDir' in code any more, as now there are only region directories...
ContributorAuthor
There was a problem hiding this comment.
Removing the check, not needed now that we are creating the resulting split region on the table dir itself.
added 2 commits
August 11, 2021 15:41
Change-Id: I8e5a207000513dd3ecab5ca807e6b85c2e39aba6
Change-Id: Iaf396b20cf742485a5bb705365712a0cdd94a915
ContributorAuthor
One thought I had now is that we probably need to back off CatalogJanitor from running whilst a split/merge is going on. As we are creating the dirs under table directory, I guess there's a risk CatalogJanitor wipe those dirs off before the proc reaches the point to add the region in meta. Let me check on this further. |
Apache9
commented
Aug 12, 2021
Contributor
Ah, yes, I also noticed this when review this PR as you mentioned CatalogJanitor. But even we do not write directly to region dir, there could still be an interval between we move the region into place and add it in meta table? I guess we should have already found the way to deal with this, otherwise it could cause data loss... |
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Aug 12, 2021
Contributor
After reviewing the code for CatalogJanitor, I do not think it will clean the partial regions on FileSystem, as I haven't seen any related code about scanning the table directory on FileSystem, it just scans the meta table. So in general, I think we should change the rollback code in SplitTableRegionProcedure and MergeTableRegionsProcedure, to remove the paritial regions when rolling back. And I think we should also have a tool in HBCK2 to remove the partial regions. Thanks. |
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Change-Id: I044bab8c9b1a4c8595a0ce6e01d73d93aa04c5bc
wchevreuil
commented
Aug 12, 2021
ContributorAuthor
Yep, had just found the same. Had added the cleanup logic on the rollback. I will work on hbck2 on a separate issue. |
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 12, 2021
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 12, 2021
💔 -1 overall
This message was automatically generated. |
Change-Id: I6ff9b9526c08c49d361460989aa0b592a2ca2abf
Apache-HBase
commented
Aug 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 13, 2021
Apache9
left a comment
Contributor
There was a problem hiding this comment.
No big concerns from me. Will give a +1 after the final small nits are resolved.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Change-Id: Ifed765ffb52dde013977efa1315d29f40fef7d8f
Apache9
approved these changes
Aug 16, 2021
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Aug 16, 2021
🎊 +1 overall
This message was automatically generated. |
Change-Id: If229fc474e22852e106568dc6d978f428cfd9884
Apache-HBase
commented
Aug 16, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 16, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 16, 2021
🎊 +1 overall
This message was automatically generated. |
wchevreuil added a commit
to wchevreuil/hbase
that referenced
this pull request
Aug 17, 2021
apache#3574) Change-Id: I574d110d6885de5eb3c8141eb6d28bb295299b3a Signed-off-by: Duo Zhang <zhangduo@apache.org>
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
apache#3574) Signed-off-by: Duo Zhang <zhangduo@apache.org> Change-Id: I738cfe24ac599a01c76a8c2ee29bb94bd021151d
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.
…d Merging
Change-Id: I5d0bbd99d27a6b3ecdd0a069dc8741239b32edfa