[GitHub] [hadoop] szetszwo commented on a change in pull request #2355: HDFS-15611. Add list Snapshot command in WebHDFS.

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

szetszwo commented on a change in pull request #2355:
URL: https://github.com/apache/hadoop/pull/2355#discussion_r498605120



##########
File path: hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java
##########
@@ -1459,6 +1460,19 @@ SnapshotDiffReport decodeResponse(Map<?, ?> json) {
     }.run();
   }
 
+  public SnapshotStatus[] getSnapshotList(final Path snapshotDir)

Review comment:
       In DistributedFileSystem, it is called
   `  public SnapshotStatus[] getSnapshotListing(Path snapshotRoot)`
   Let's use the same name?
   

##########
File path: hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotStatus.java
##########
@@ -25,6 +25,7 @@
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.DFSUtilClient;
+import org.apache.hadoop.util.StringUtils;

Review comment:
       Unused import.

##########
File path: hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/client/HttpFSFileSystem.java
##########
@@ -1582,6 +1584,18 @@ public SnapshotDiffReport getSnapshotDiffReport(Path path,
     return JsonUtilClient.toSnapshottableDirectoryList(json);
   }
 
+  public SnapshotStatus[] getSnapshotList(Path snapshotRoot)

Review comment:
       It should call getSnapshotListing.

##########
File path: hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/JsonUtilClient.java
##########
@@ -872,4 +873,39 @@ private static SnapshottableDirectoryStatus toSnapshottableDirectoryStatus(
             snapshotQuota, parentFullPath);
     return snapshottableDirectoryStatus;
   }
+
+  public static SnapshotStatus[] toSnapshotList(final Map<?, ?> json) {
+    if (json == null) {
+      return null;
+    }
+    List<?> list = (List<?>) json.get("SnapshotList");
+    if (list == null) {
+      return null;
+    }
+    SnapshotStatus[] statuses =
+        new SnapshotStatus[list.size()];
+    for (int i = 0; i < list.size(); i++) {
+      statuses[i] = toSnapshotStatus((Map<?, ?>) list.get(i));
+    }
+    return statuses;
+  }
+
+  private static SnapshotStatus toSnapshotStatus(
+      Map<?, ?> json) {
+    if (json == null) {
+      return null;
+    }
+    int snapshotID = getInt(json, "snapshotID", 0);
+    boolean isDeleted = ((String)json.get("deletionStatus")).
+        contentEquals("DELETED");

Review comment:
       Use "DELETED".equal(..) in order to avoid NPE?
   ` final boolean isDeleted = "DELETED".equals(json.get("deletionStatus"));`
   




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