-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29662 - Avoid regionDir/tableDir creation as part of .regioninfo file creation #7406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8cf73b6 to
33362aa
Compare
| FsPermission perms = CommonFSUtils.getFilePermissions(fs, conf, HConstants.DATA_FILE_UMASK_KEY); | ||
| // Write the RegionInfo file content | ||
| try (FSDataOutputStream out = FSUtils.create(conf, fs, regionInfoFile, perms, null)) { | ||
| try (FSDataOutputStream out = FSUtils.create(conf, fs, regionInfoFile, perms, null, false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here why this change, providing the JIRA identifier. The reason not to recursively create directories is important, we don't want someone refactoring this code later to miss that.
|
I know it's difficult but any possibility of creating some tests around this? |
+1, as discussed offline this can actually be tested with same scenario @gvprathyusha6
|
33362aa to
d482b64
Compare
@ujjawal4046 why not just add the test for HRegion initialise to fail if there is no regionDir? that way all downstreams are covered? |
d482b64 to
cdf26b3
Compare
cdf26b3 to
8bd7056
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8bd7056 to
bc2f551
Compare
This comment has been minimized.
This comment has been minimized.
0579809 to
c2cb21c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c2cb21c to
d8657bc
Compare
This comment has been minimized.
This comment has been minimized.
|
lgtm, I plan to apply this back to 2.5 tomorrow, unless objection. |
virajjasani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few minor comments, looks good overall
| mfs.getFileSystem(), tableDir, regionInfo); | ||
| } catch (IOException e) { | ||
| LOG.warn("Failed to create region directory for {}: {}", | ||
| regionInfo.getRegionNameAsString(), e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's print whole stacktrace here instead of error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if we suppress the error here, how can fixHoles progress? I hope that's the only place using this utility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can fixHoles progress
It shall continue to fix hole for other regions, right now also that part is taken care of by the procedure logic, as part of Assign it is creating the region directory, so if region directory creation fails, it would only effect that region and not rest of them, so I tried to keep it similar
I hope that's the only place using this utility?
yes and its private
| throw new IOException("Region directory does not exist: " + getRegionDir()); | ||
| } | ||
| if (!fs.exists(getTempDir())) { | ||
| fs.mkdirs(getTempDir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, this was not covered so far?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was created previously as part of recursion in FS while creating file, now that I removed it and now we dont just blindly create all the directories that are present in path, we would need to create tmp directory explicitly
…o file creation in HRegion initialize
d8657bc to
5bce7ca
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…o file creation in HRegion initialize (#7406) Signed-off-by: Andrew Purtell <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
…o file creation in HRegion initialize (#7406) Signed-off-by: Andrew Purtell <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/MetaFixer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
…o file creation in HRegion initialize (#7406) Signed-off-by: Andrew Purtell <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/MetaFixer.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
…o file creation in HRegion initialize (#7406) Signed-off-by: Andrew Purtell <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/MetaFixer.java hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TruncateRegionProcedure.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
The test added currently fails on master branch and empty regionDir/regioninfo file are still created, with the fix it is throwing an excepted exception .