[GitHub] [hbase] wchevreuil commented on a diff in pull request #5369: HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem

["wchevreuil (via GitHub)" <gi...@apache.org>]

wchevreuil commented on code in PR #5369:
URL: https://github.com/apache/hbase/pull/5369#discussion_r1304301738


##########
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java:
##########
@@ -158,6 +158,7 @@ public void testGrantGlobal1() throws Exception {
 
     TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table);
     snapshotAndWait(snapshot1, table);
+    snapshotAndWait(snapshot2, table);

Review Comment:
   Does this test really need this change, or is this part of the PR by mistake?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java:
##########
@@ -432,6 +430,40 @@ public static void completeSnapshot(Path snapshotDir, Path workingDir, FileSyste
     }
   }
 
+  static boolean shouldSkipRenameSnapshotDirectories(URI workingURI, URI rootURI) {
+    // check scheme, e.g. file, hdfs
+    if (
+      workingURI.getScheme() == null
+        && (rootURI.getScheme() != null && !rootURI.getScheme().equalsIgnoreCase("file"))

Review Comment:
   Why we need the extra check for the "file" URI? Wouldn't it be enough to return "true" here if workingURI scheme is null and rootURI is not?



-- 
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