[GitHub] [hbase] d-c-manning commented on a diff in pull request #5437: HBASE-28081 Snapshot working dir does not retain ACLs after snapshot commit phase

["d-c-manning (via GitHub)" <gi...@apache.org>]

d-c-manning commented on code in PR #5437:
URL: https://github.com/apache/hbase/pull/5437#discussion_r1339066065


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java:
##########
@@ -573,6 +576,36 @@ public synchronized void prepareWorkingDirectory(SnapshotDescription snapshot)
     }
   }
 
+  /**
+   * If the parent dir of the snapshot working dir (e.g. /hbase/.hbase-snapshot) has non-empty ACLs,
+   * use them for the current working dir (e.g. /hbase/.hbase-snapshot/.tmp/{snapshot-name}) so that
+   * regardless of whether the snapshot commit phase performs atomic rename or non-atomic copy of
+   * the working dir to new snapshot dir, the ACLs are retained.
+   * @param workingDir   working dir to build the snapshot.
+   * @param workingDirFS working dir file system.
+   * @throws IOException If ACL read/modify operation fails.
+   */
+  private static void updateWorkingDirAclsIfRequired(Path workingDir, FileSystem workingDirFS)
+    throws IOException {
+    AclStatus snapshotWorkingParentDirStatus;
+    try {
+      snapshotWorkingParentDirStatus =
+        workingDirFS.getAclStatus(workingDir.getParent().getParent());

Review Comment:
   is `workingDir` guaranteed to have a parent of a parent to avoid any possible NullPointerException? Or could `workingDir` theoretically be `/` or `/tmp`?



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org