[GitHub] [hadoop] smengcl commented on a change in pull request #2682: HDFS-15820. Ensure snapshot root trash provisioning happens only post safe mode exit

[GitBox <gi...@apache.org>]

smengcl commented on a change in pull request #2682:
URL: https://github.com/apache/hadoop/pull/2682#discussion_r570461526



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##########
@@ -8531,25 +8527,37 @@ void checkAccess(String src, FsAction mode) throws IOException {
    * Check if snapshot roots are created for all existing snapshottable
    * directories. Create them if not.
    */
-  void checkAndProvisionSnapshotTrashRoots() throws IOException {
-    SnapshottableDirectoryStatus[] dirStatusList = getSnapshottableDirListing();
-    if (dirStatusList == null) {
-      return;
-    }
-    for (SnapshottableDirectoryStatus dirStatus : dirStatusList) {
-      String currDir = dirStatus.getFullPath().toString();
-      if (!currDir.endsWith(Path.SEPARATOR)) {
-        currDir += Path.SEPARATOR;
-      }
-      String trashPath = currDir + FileSystem.TRASH_PREFIX;
-      HdfsFileStatus fileStatus = getFileInfo(trashPath, false, false, false);
-      if (fileStatus == null) {
-        LOG.info("Trash doesn't exist for snapshottable directory {}. "
-            + "Creating trash at {}", currDir, trashPath);
-        PermissionStatus permissionStatus = new PermissionStatus(getRemoteUser()
-            .getShortUserName(), null, SHARED_TRASH_PERMISSION);
-        mkdirs(trashPath, permissionStatus, false);
+  @Override
+  public void checkAndProvisionSnapshotTrashRoots() {
+    if (isSnapshotTrashRootEnabled) {
+      try {
+        SnapshottableDirectoryStatus[] dirStatusList =
+            getSnapshottableDirListing();
+        if (dirStatusList == null) {
+          return;
+        }
+        for (SnapshottableDirectoryStatus dirStatus : dirStatusList) {
+          String currDir = dirStatus.getFullPath().toString();
+          if (!currDir.endsWith(Path.SEPARATOR)) {
+            currDir += Path.SEPARATOR;
+          }
+          String trashPath = currDir + FileSystem.TRASH_PREFIX;
+          HdfsFileStatus fileStatus = getFileInfo(trashPath, false, false, false);
+          if (fileStatus == null) {
+            LOG.info("Trash doesn't exist for snapshottable directory {}. " + "Creating trash at {}", currDir, trashPath);
+            PermissionStatus permissionStatus =
+                new PermissionStatus(getRemoteUser().getShortUserName(), null,
+                    SHARED_TRASH_PERMISSION);
+            mkdirs(trashPath, permissionStatus, false);
+          }
+        }
+      } catch (IOException e) {
+        final String msg =
+            "Could not provision Trash directory for existing "
+                + "snapshottable directories. Exiting Namenode.";
+        ExitUtil.terminate(1, msg);

Review comment:
       Pro: Terminating NN in this case is a sure good way of uncovering an unexpected problems instead of hiding it in the logs.
   
   Con: I wonder if we really should terminate NN when Trash directory fails to be deployed. We could just throw a warning message?
   
   Either way, I'm fine with both. Just a thought.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
##########
@@ -2524,7 +2524,7 @@ public void testNameNodeCreateSnapshotTrashRootOnStartup()
     MiniDFSCluster cluster =
         new MiniDFSCluster.Builder(conf).numDataNodes(1).build();
     try {
-      final DistributedFileSystem dfs = cluster.getFileSystem();
+     DistributedFileSystem dfs = cluster.getFileSystem();

Review comment:
       nit: add one more space before this line for alignment.




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org