HBASE-22578 HFileCleaner should not delete empty ns/table directories… #337
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
jatsakthi
requested changes
Jun 27, 2019
| * acls are set at these directories; the archive data directory should not be deleted because | ||
| * the HDFS acls of global permission is set at this directory. | ||
| */ | ||
| /*if (userScanSnapshotEnabled) { |
There was a problem hiding this comment.
This if condition shouldn't be commented. Without it, the added test case "testCleanArchiveTableDir" fails.
There was a problem hiding this comment.
Yes, sorry for my carelessness, will fix it.
mymeiyi
force-pushed
the
hdfs-acl-cleaner
branch
from
fc69c10
to
cd08e01
Compare
|
🎊 +1 overall
This message was automatically generated. |
mymeiyi
force-pushed
the
hdfs-acl-cleaner
branch
from
cd08e01
to
c93a21d
Compare
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Jun 28, 2019
| * @param dir The path of a directory | ||
| * @return True if the directory can be deleted, otherwise false | ||
| */ | ||
| boolean shouldDeleteDirectory(Path dir) { |
| * @param dir Path of the directory | ||
| * @return True if the directory can be deleted, otherwise false | ||
| */ | ||
| private boolean canDeleteEmptyDirectory(Path dir) { |
There was a problem hiding this comment.
canDeleteEmptyDirectory -> isEmptyDirDeletable ?
|
|
||
| private boolean isUserScanSnapshotEnabled(Configuration conf) { | ||
| String masterCoprocessors = conf.get(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); | ||
| if (conf.getBoolean(SnapshotScannerHDFSAclHelper.USER_SCAN_SNAPSHOT_ENABLE, false) |
There was a problem hiding this comment.
Reconsider the USER_SCAN_SNAPSHOT_ENABLE name, seems not a good name, because even if we don't enable the feature, user can still scan the snapshot ? so better to use SYNC_TABLE_ACL_IN_HDFS or some other config keys to express the point is to sync the table granted acl to HDFS acl, it will help to understand this.
BTW, a deprecated config keys is a trouble, so better to give a suitable name in the first definition.
There was a problem hiding this comment.
Yes, great suggestions, I think there are some configs should be renamed to a more proper name, I will do this work in another issus.
| return false; | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.warn("Check if namespace {} exists error", namespace, e); |
There was a problem hiding this comment.
Just throw the exception ? and handle in the outside (CleanerChore#canDeleteEmptyDirectory )
| } | ||
|
|
||
| @Override | ||
| public boolean canDeleteEmptyDirectory(Path dir) { |
There was a problem hiding this comment.
isEmptyDirDeletable ? or isDirDeletable ?
|
|
||
| @VisibleForTesting | ||
| static boolean isArchiveNamespaceDir(Path path) { | ||
| return path == null ? false : isArchiveDataDir(path.getParent()); |
There was a problem hiding this comment.
return path != null && isArchiveDataDir(path.getParent()) ?
|
|
||
| @VisibleForTesting | ||
| static boolean isArchiveTableDir(Path path) { | ||
| return path == null ? false : isArchiveNamespaceDir(path.getParent()); |
mymeiyi
force-pushed
the
hdfs-acl-cleaner
branch
2 times, most recently
from
2e0e1b0
to
50aac57
Compare
openinx
approved these changes
Jun 28, 2019
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
… used for user san snapshot feature
mymeiyi
force-pushed
the
hdfs-acl-cleaner
branch
from
50aac57
to
a00149d
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Jul 25, 2019
… used for user san snapshot feature (#337)
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
… used for user san snapshot feature (apache#337)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
… used for user san snapshot feature