virajjasani commented on code in PR #4391:
URL: https://github.com/apache/hbase/pull/4391#discussion_r866360992
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##########
@@ -355,9 +354,9 @@ private void checkStagingDir() throws IOException {
if (!this.fs.mkdirs(p, HiddenDirPerms)) {
throw new IOException("Failed to create staging directory " +
p.toString());
}
- } else {
- this.fs.setPermission(p, HiddenDirPerms);
}
+ this.fs.setPermission(p, HiddenDirPerms);
Review Comment:
In this case, `staging` dir would have already been created with
`HiddenDirPerms` perf i.e. `this.fs.mkdirs(p, HiddenDirPerms)` should be true
for this statement to be executed. So, isn't this statement redundant? We
already created staging dir with HiddenDirPerms permission and now we are again
setting `HiddenDirPerms` to `staging` dir.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java:
##########
@@ -331,16 +331,15 @@ private void checkSubDir(final Path p, final String
dirPermsConfName) throws IOE
throw new IOException("HBase directory '" + p + "' creation
failure.");
}
}
- } else {
- if (isSecurityEnabled &&
!dirPerms.equals(fs.getFileStatus(p).getPermission())) {
- // check whether the permission match
- LOG.warn("Found HBase directory permissions NOT matching expected
permissions for "
- + p.toString() + " permissions=" +
fs.getFileStatus(p).getPermission() + ", expecting "
- + dirPerms + ". Automatically setting the permissions. "
- + "You can change the permissions by setting \"" + dirPermsConfName
- + "\" in hbase-site.xml " + "and restarting the master");
- fs.setPermission(p, dirPerms);
- }
+ }
+ if (isSecurityEnabled &&
!dirPerms.equals(fs.getFileStatus(p).getPermission())) {
Review Comment:
This part looks good because here we check permission regardless of how/when
dir was created.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
