Uh oh!
There was an error while loading. Please reload this page.
HBASE-22617 Recovered WAL directories not getting cleaned up - #330
Merged
Conversation
Apache-HBase
commented
Jun 22, 2019
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Jun 23, 2019
ContributorAuthor
Fixed the failed UTs. In the new patch, will delete the wrong region wal directory after successfully opening a region. And also, in GCRegionProcedure we will delete the region wal directory, and in DeleteTableProcedure.deleteFromFs(which will be called from many places), we will delete the table wal directory. |
Apache-HBase
commented
Jun 23, 2019
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Jun 24, 2019
ContributorAuthor
Let me check the failed UTs. |
Apache9
commented
Jun 24, 2019
ContributorAuthor
TestHbck.testRecoverSplitAfterMetaUpdated is still failing. Will dig more. |
infraio
reviewed
Jun 24, 2019
| */ | ||
| public Path getRegionDir(RegionInfo region) { | ||
| return FSUtils.getRegionDir(FSUtils.getTableDir(getRootDir(), region.getTable()), region); | ||
| return FSUtils.getRegionDirFromRootDir(getRootDir(), region); |
Contributor
There was a problem hiding this comment.
Why this? The old impl should be getRegionDirFromTableDir?
ContributorAuthor
There was a problem hiding this comment.
They are the same...
You can see the implementation of getRegionDirFromRootDir, it will first generate the tableDir...
| // Cleanup the directories on WAL filesystem also | ||
| Path regionWALDir = FSUtils.getWALRegionDir(env.getMasterConfiguration(), | ||
| getRegion().getTable(), getRegion().getEncodedName()); | ||
| walFs.delete(regionWALDir, true); |
Contributor
There was a problem hiding this comment.
regionWALDir should already be deleted when archive region dir?
ContributorAuthor
There was a problem hiding this comment.
They could be on different file system. The table data are on the normal file system and these are on the wal file system. By default they are the same but they could be different, i.e, when deploying HBase on S3.
Contributor
There was a problem hiding this comment.
Got it. But for the wrong regionWALDir, how about exist first and then delete it?
| // Delete the directory on wal filesystem | ||
| FileSystem walFs = mfs.getWALFileSystem(); | ||
| Path tableWALDir = FSUtils.getWALTableDir(env.getMasterConfiguration(), tableName); |
Contributor
There was a problem hiding this comment.
This should already be deleted when archive table?
ContributorAuthor
There was a problem hiding this comment.
They could be on different file system. The table data are on the normal file system and these are on the wal file system. By default they are the same but they could be different, i.e, when deploying HBase on S3.
| if (!createDir(splitdir)) { | ||
| throw new IOException("Failed create of " + splitdir); | ||
| } | ||
| Path daughterATmpDir = getSplitsDir(daughterA); |
ContributorAuthor
There was a problem hiding this comment.
We have to create the directories for daughter regions here, as it is possible that we do not have any store files to split, which means the directories may not be created when we call commitDaugherRegion, and then we will miss the .regioninfo file and cause TestHbck to fail. Not sure why this does not have problem in the past but it is no harm...
Apache-HBase
commented
Jun 24, 2019
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Jun 24, 2019
Uh oh!
There was an error while loading. Please reload this page.
infraio
reviewed
Jun 24, 2019
Uh oh!
There was an error while loading. Please reload this page.
infraio
reviewed
Jun 24, 2019
Uh oh!
There was an error while loading. Please reload this page.
infraio
reviewed
Jun 24, 2019
Uh oh!
There was an error while loading. Please reload this page.
infraio
reviewed
Jun 24, 2019
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jun 24, 2019
💔 -1 overall
This message was automatically generated. |
infraio
approved these changes
Jun 24, 2019
Apache-HBase
commented
Jun 24, 2019
💔 -1 overall
This message was automatically generated. |
apurtell
approved these changes
Jun 24, 2019
apurtell
left a comment
Contributor
There was a problem hiding this comment.
lgtm assuming all tests pass.
I may be back. Using this to make a branch-1 patch. Might find a small detail later.
| // Cleanup the directories on WAL filesystem also | ||
| Path regionWALDir = FSUtils.getWALRegionDir(env.getMasterConfiguration(), | ||
| getRegion().getTable(), getRegion().getEncodedName()); | ||
| walFs.delete(regionWALDir, true); |
| } | ||
| } | ||
| for (Path file: files) { | ||
| for (Path file : filesUnderWrongRegionWALDir) { |
Contributor
There was a problem hiding this comment.
Small code smell. Now that we have three lists, why not create one List, add to the one lists the files we find in any of the three locations we must check, then process the one list all at once. The only difference here is the log messages are slightly different but operators and us won't care if "root" or "wrong". The error or debug log lines all contain the file path, which is enough, I think.
ContributorAuthor
There was a problem hiding this comment.
They are on different file systems so at least we need two loops...
| return new Path(tabledir, name); | ||
| } | ||
| /** |
apurtell
commented
Jun 24, 2019
Contributor
After the fix is ready we should also have a unit test that confirms that no files are written outside of where we expect, so we can catch this kind of error in the future. Could be done as part of this work or as a follow up PR and JIRA, but we should have that additional test coverage in place going forward. |
| Path tableWALDir = FSUtils.getWALTableDir(env.getMasterConfiguration(), tableName); | ||
| if (walFs.exists(tableWALDir) && !walFs.delete(tableWALDir, true)) { | ||
| throw new IOException("Couldn't delete table dir on wal filesystem" + tableWALDir); | ||
| } |
Contributor
There was a problem hiding this comment.
Should we not be checking for wrong wal directory here too and deleting that if it exists?
ContributorAuthor
There was a problem hiding this comment.
Not necessary I think, at least there is no way to delete the namespace directories... So the users hava to use a script to delete the empty directories manually...
apurtell
approved these changes
Jun 24, 2019
| // Cleanup the directories on WAL filesystem also | ||
| Path regionWALDir = FSUtils.getWALRegionDir(env.getMasterConfiguration(), | ||
| getRegion().getTable(), getRegion().getEncodedName()); | ||
| walFs.delete(regionWALDir, true); |
Contributor
There was a problem hiding this comment.
Please log at DEBUG if delete fails
| walFs.delete(regionWALDir, true); | ||
| Path wrongRegionWALDir = FSUtils.getWrongWALRegionDir(env.getMasterConfiguration(), | ||
| getRegion().getTable(), getRegion().getEncodedName()); | ||
| walFs.delete(wrongRegionWALDir, true); |
Contributor
There was a problem hiding this comment.
Please log at DEBUG if delete fails
| getRegionInfo().getEncodedName()); | ||
| FileSystem walFs = getWalFileSystem(); | ||
| if (walFs.exists(wrongRegionWALDir)) { | ||
| walFs.delete(wrongRegionWALDir, true); |
Contributor
There was a problem hiding this comment.
Please log at DEBUG if delete fails
Apache9
commented
Jun 25, 2019
ContributorAuthor
Let's do this in a follow up, as the 'outside of where we expect' is not a clear description, we need to discuss how to implement the test, as well as how to check the result. Let me address the review comments. |
Apache-HBase
commented
Jun 25, 2019
💔 -1 overall
This message was automatically generated. |
asfgit pushed a commit
that referenced
this pull request
Jun 25, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Jun 25, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Jun 25, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Jun 25, 2019
Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
| for (Path file: files) { | ||
| if (!walFS.delete(file, false)) { | ||
| LOG.error("Failed delete of " + file); | ||
| for (Path file : filesUnderRootDir) { |
There was a problem hiding this comment.
hi, if the region is read-only mode for HbaseTableSnapshotInputFormat, it try to delete the filesUnderRootDir and no access, it will throw exception and failed
symat pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
) Signed-off-by: Guanghao Zhang <zghao@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 2b7a7da) Change-Id: I01752de3d258ac5e68fe0cd735bafadedb7547c3
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
6 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.
No description provided.