Addressed checkstyle/findbug as well as review comments.

bshashikant · bshashikant · commit 04f02d2d9820 · 2020-07-27T12:01:07.000+05:30
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml b/hadoop-hdfs-project/hadoop-hdfs-client/dev-support/findbugsExcludeFile.xml
@@ -22,6 +22,8 @@
       <Class name="org.apache.hadoop.hdfs.util.StripedBlockUtil$ChunkByteArray"/>
       <Class name="org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing$DiffReportListingEntry"/>
       <Class name="org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing"/>
+      <Class name="org.apache.hadoop.hdfs.protocol.SnapshotStatus"/>
+    </Or>
     </Or>
     <Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2" />
   </Match>
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
@@ -2192,16 +2192,16 @@ public SnapshottableDirectoryStatus[] getSnapshottableDirListing()
   }
 
   /**
-   * Get listing of all the snapshots for a snapshottable directory
+   * Get listing of all the snapshots for a snapshottable directory.
    *
    * @return Information about all the snapshots for a snapshottable directory
    * @throws IOException If an I/O error occurred
-   * @see ClientProtocol#getSnapshotListing()
+   * @see ClientProtocol#getSnapshotListing(String)
    */
   public SnapshotStatus[] getSnapshotListing(String snapshotRoot)
       throws IOException {
     checkOpen();
-    try (TraceScope ignored = tracer.newScope("getSnapshottableDirListing")) {
+    try (TraceScope ignored = tracer.newScope("getSnapshotListing")) {
       return namenode.getSnapshotListing(snapshotRoot);
     } catch (RemoteException re) {
       throw re.unwrapRemoteException();
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSOpsCountStatistics.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSOpsCountStatistics.java
@@ -111,6 +111,7 @@ public enum OpType {
     SET_XATTR("op_set_xattr"),
     GET_SNAPSHOT_DIFF("op_get_snapshot_diff"),
     GET_SNAPSHOTTABLE_DIRECTORY_LIST("op_get_snapshottable_directory_list"),
+    GET_SNAPSHOT_LIST("op_get_snapshot_list"),
     TRUNCATE(CommonStatisticNames.OP_TRUNCATE),
     UNSET_EC_POLICY("op_unset_ec_policy"),
     UNSET_STORAGE_POLICY("op_unset_storage_policy");
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
@@ -2155,7 +2155,11 @@ public SnapshottableDirectoryStatus[] getSnapshottableDirListing()
    */
   public SnapshotStatus[] getSnapshotListing(Path snapshotRoot)
       throws IOException {
-    return dfs.getSnapshotListing(getPathName(snapshotRoot));
+    Path absF = fixRelativePart(snapshotRoot);
+    statistics.incrementReadOps(1);
+    storageStatistics
+        .incrementOpCounter(OpType.GET_SNAPSHOT_LIST);
+    return dfs.getSnapshotListing(getPathName(absF));
   }
 
   @Override
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
@@ -728,7 +728,7 @@ SnapshottableDirectoryStatus[] getSnapshottableDirListing()
       throws IOException;
 
   /**
-   * Get listing of all the snapshots for a snapshottable directory
+   * Get listing of all the snapshots for a snapshottable directory.
    *
    * @return Information about all the snapshots for a snapshottable directory
    * @throws IOException If an I/O error occurred
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotStatus.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotStatus.java
@@ -27,16 +27,16 @@
 import org.apache.hadoop.hdfs.DFSUtilClient;
 
 /**
- * Metadata about a snapshottable directory
+ * Metadata about a snapshottable directory.
  */
 public class SnapshotStatus {
   /**
-   * Basic information of the snapshot directory
+   * Basic information of the snapshot directory.
    */
   private final HdfsFileStatus dirStatus;
 
   /**
-   * Snapshot ID for the snapshot
+   * Snapshot ID for the snapshot.
    */
   private final int snapshotID;
 
@@ -45,16 +45,16 @@ public class SnapshotStatus {
    */
   private final byte[] parentFullPath;
 
-  public SnapshotStatus(long modification_time, long access_time,
+  public SnapshotStatus(long modificationTime, long accessTime,
                         FsPermission permission,
                         EnumSet<HdfsFileStatus.Flags> flags,
                         String owner, String group, byte[] localName,
                         long inodeId, int childrenNum, int snapshotID,
                         byte[] parentFullPath) {
     this.dirStatus = new HdfsFileStatus.Builder()
         .isdir(true)
-        .mtime(modification_time)
-        .atime(access_time)
+        .mtime(modificationTime)
+        .atime(accessTime)
         .perm(permission)
         .flags(flags)
         .owner(owner)
@@ -102,9 +102,9 @@ public Path getFullPath() {
     String parentFullPathStr =
         (parentFullPath == null || parentFullPath.length == 0) ?
             "/" : DFSUtilClient.bytes2String(parentFullPath);
-      return new Path(getSnapshotPath(parentFullPathStr,
-          dirStatus.getLocalName()));
-    }
+    return new Path(getSnapshotPath(parentFullPathStr,
+        dirStatus.getLocalName()));
+  }
 
   /**
    * Print a list of {@link SnapshotStatus} out to a given stream.
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java
@@ -1674,8 +1674,9 @@ public static SnapshottableDirectoryStatus convert(
 
   public static SnapshotStatus[] convert(
       HdfsProtos.SnapshotListingProto sdlp) {
-    if (sdlp == null)
+    if (sdlp == null) {
       return null;
+    }
     List<HdfsProtos.SnapshotStatusProto> list = sdlp
         .getSnapshotListingList();
     if (list.isEmpty()) {
@@ -2712,8 +2713,9 @@ public static SnapshottableDirectoryListingProto convert(
 
   public static HdfsProtos.SnapshotListingProto convert(
       SnapshotStatus[] status) {
-    if (status == null)
+    if (status == null) {
       return null;
+    }
     HdfsProtos.SnapshotStatusProto[] protos =
         new HdfsProtos.SnapshotStatusProto[status.length];
     for (int i = 0; i < status.length; i++) {
diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterSnapshot.java
@@ -163,15 +163,17 @@ public SnapshotStatus[] getSnapshotListing(String snapshotRoot)
     rpcServer.checkOperation(NameNode.OperationCategory.READ);
     final List<RemoteLocation> locations =
         rpcServer.getLocationsForPath(snapshotRoot, true, false);
-    RemoteMethod method = new RemoteMethod("getSnapshotListing",
-        new Class<?>[] {String.class},
+    RemoteMethod remoteMethod = new RemoteMethod("getSnapshotListing",
+        new Class<?>[]{String.class},
         new RemoteParam());
-    Set<FederationNamespaceInfo> nss = namenodeResolver.getNamespaces();
-    Map<FederationNamespaceInfo, SnapshotStatus[]> ret =
-        rpcClient.invokeConcurrent(
-            nss, method, true, false, SnapshotStatus[].class);
-
-    return RouterRpcServer.merge(ret, SnapshotStatus.class);
+    if (rpcServer.isInvokeConcurrent(snapshotRoot)) {
+      Map<RemoteLocation, SnapshotStatus[]> ret = rpcClient.invokeConcurrent(
+          locations, remoteMethod, true, false, SnapshotStatus[].class);
+      return ret.values().iterator().next();
+    } else {
+      return rpcClient.invokeSequential(
+          locations, remoteMethod, SnapshotStatus[].class, null);
+    }
   }
 
   public SnapshotDiffReport getSnapshotDiffReport(String snapshotRoot,
diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java
@@ -71,29 +71,9 @@
 import org.apache.hadoop.hdfs.MiniDFSCluster.DataNodeProperties;
 import org.apache.hadoop.hdfs.NameNodeProxies;
 import org.apache.hadoop.hdfs.client.HdfsDataOutputStream;
-import org.apache.hadoop.hdfs.protocol.AddErasureCodingPolicyResponse;
-import org.apache.hadoop.hdfs.protocol.BlockStoragePolicy;
-import org.apache.hadoop.hdfs.protocol.CacheDirectiveInfo;
-import org.apache.hadoop.hdfs.protocol.CachePoolEntry;
-import org.apache.hadoop.hdfs.protocol.CachePoolInfo;
-import org.apache.hadoop.hdfs.protocol.ClientProtocol;
-import org.apache.hadoop.hdfs.protocol.DatanodeInfo;
-import org.apache.hadoop.hdfs.protocol.DirectoryListing;
-import org.apache.hadoop.hdfs.protocol.ECBlockGroupStats;
-import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicy;
-import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicyInfo;
-import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicyState;
-import org.apache.hadoop.hdfs.protocol.HdfsConstants;
+import org.apache.hadoop.hdfs.protocol.*;
 import org.apache.hadoop.hdfs.protocol.HdfsConstants.DatanodeReportType;
 import org.apache.hadoop.hdfs.protocol.HdfsConstants.SafeModeAction;
-import org.apache.hadoop.hdfs.protocol.HdfsFileStatus;
-import org.apache.hadoop.hdfs.protocol.LocatedBlock;
-import org.apache.hadoop.hdfs.protocol.LocatedBlocks;
-import org.apache.hadoop.hdfs.protocol.ReplicatedBlockStats;
-import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport;
-import org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing;
-import org.apache.hadoop.hdfs.protocol.SnapshotException;
-import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus;
 import org.apache.hadoop.hdfs.security.token.block.ExportedBlockKeys;
 import org.apache.hadoop.hdfs.server.blockmanagement.BlockManager;
 import org.apache.hadoop.hdfs.server.blockmanagement.BlockManagerTestUtil;
@@ -110,6 +90,7 @@
 import org.apache.hadoop.hdfs.server.namenode.INodeDirectory;
 import org.apache.hadoop.hdfs.server.namenode.NameNode;
 import org.apache.hadoop.hdfs.server.namenode.NameNodeAdapter;
+import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotTestHelper;
 import org.apache.hadoop.hdfs.server.protocol.BlocksWithLocations;
 import org.apache.hadoop.hdfs.server.protocol.BlocksWithLocations.BlockWithLocations;
 import org.apache.hadoop.hdfs.server.protocol.DatanodeStorageReport;
@@ -926,6 +907,16 @@ public void testGetSnapshotListing() throws IOException {
     SnapshottableDirectoryStatus snapshotDir0 = dirList[0];
     assertEquals(snapshotPath, snapshotDir0.getFullPath().toString());
 
+    // check for snapshot listing through the Router
+    SnapshotStatus[] snapshots = routerProtocol.
+        getSnapshotListing(snapshotPath);
+    assertEquals(2, snapshots.length);
+    assertEquals(SnapshotTestHelper.getSnapshotRoot
+        (new Path(snapshotPath), snapshot1),
+        snapshots[0].getFullPath());
+    assertEquals(SnapshotTestHelper.getSnapshotRoot
+        (new Path(snapshotPath), snapshot2),
+        snapshots[1].getFullPath());
     // Check for difference report in two snapshot
     SnapshotDiffReport diffReport = routerProtocol.getSnapshotDiffReport(
         snapshotPath, snapshot1, snapshot2);
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
@@ -156,15 +156,14 @@ static SnapshottableDirectoryStatus[] getSnapshottableDirListing(
   }
 
   static SnapshotStatus[] getSnapshotListing(
-      FSDirectory fsd, SnapshotManager snapshotManager, String path)
+      FSDirectory fsd, FSPermissionChecker pc, SnapshotManager snapshotManager,
+      String path)
       throws IOException {
-    FSPermissionChecker pc = fsd.getPermissionChecker();
     fsd.readLock();
     try {
       INodesInPath iip = fsd.getINodesInPath(path, DirOp.READ);
       if (fsd.isPermissionEnabled()) {
-        fsd.checkPermission(pc, iip, false, null, null, FsAction.READ,
-            FsAction.READ);
+        fsd.checkPathAccess(pc, iip, FsAction.READ);
       }
       return snapshotManager.getSnapshotListing(iip);
     } finally {
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
@@ -7011,22 +7011,27 @@ public SnapshottableDirectoryStatus[] getSnapshottableDirListing()
    */
   public SnapshotStatus[] getSnapshotListing(String snapshotRoot)
       throws IOException {
-    SnapshotStatus[] status = null;
+    final String operationName = "listSnapshotDirectory";
+    SnapshotStatus[] status;
     checkOperation(OperationCategory.READ);
     boolean success = false;
-    readLock();
+    final FSPermissionChecker pc = getPermissionChecker();
+    FSPermissionChecker.setOperationType(operationName);
     try {
-      checkOperation(OperationCategory.READ);
-      status = FSDirSnapshotOp.getSnapshotListing(dir, snapshotManager,
-          snapshotRoot);
-      success = true;
+      readLock();
+      try {
+        checkOperation(OperationCategory.READ);
+        status = FSDirSnapshotOp.getSnapshotListing(dir, pc, snapshotManager,
+            snapshotRoot);
+        success = true;
+      } finally {
+        readUnlock(operationName, getLockReportInfoSupplier(null));
+      }
     } catch (AccessControlException ace) {
-      logAuditEvent(success, "listSnapshots", null, null, null);
+      logAuditEvent(success, "listSnapshots", snapshotRoot);
       throw ace;
-    } finally {
-      readUnlock();
     }
-    logAuditEvent(success, "listSnapshots", null, null, null);
+    logAuditEvent(success, "listSnapshots", snapshotRoot);
     return status;
   }
   /**
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
@@ -2006,11 +2006,11 @@ public SnapshottableDirectoryStatus[] getSnapshottableDirListing()
   }
 
   @Override // Client Protocol
-  public SnapshotStatus[] getSnapshotListing(String path)
+  public SnapshotStatus[] getSnapshotListing(String snapshotRoot)
       throws IOException {
     checkNNStartup();
     SnapshotStatus[] status = namesystem
-        .getSnapshotListing(path);
+        .getSnapshotListing(snapshotRoot);
     metrics.incrListSnapshotsOps();
     return status;
   }
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
@@ -516,26 +516,22 @@ public SnapshottableDirectoryStatus[] getSnapshottableDirListing(
   public SnapshotStatus[] getSnapshotListing(INodesInPath iip)
       throws IOException {
     INodeDirectory srcRoot = getSnapshottableRoot(iip);
-    ReadOnlyList<Snapshot> snapshotList = srcRoot.getDirectorySnapshottableFeature().
-        getSnapshotList();
-    if (snapshotList.isEmpty()) {
-      return null;
-    }
-    List<SnapshotStatus> statusList =
-        new ArrayList<>();
-    for (Snapshot s : snapshotList) {
+    ReadOnlyList<Snapshot> snapshotList = srcRoot.
+        getDirectorySnapshottableFeature().getSnapshotList();
+    SnapshotStatus[] statuses = new SnapshotStatus[snapshotList.size()];
+    for (int count = 0; count < snapshotList.size(); count++) {
+      Snapshot s = snapshotList.get(count);
       Snapshot.Root dir = s.getRoot();
-      SnapshotStatus status = new SnapshotStatus(dir.getModificationTime()
+      statuses[count] = new SnapshotStatus(dir.getModificationTime()
           , dir.getAccessTime(), dir.getFsPermission(),
           EnumSet.noneOf(HdfsFileStatus.Flags.class),
           dir.getUserName(), dir.getGroupName(),
           dir.getLocalNameBytes(), dir.getId(),
           dir.getChildrenNum(Snapshot.CURRENT_STATE_ID),
           s.getId(), DFSUtil.string2Bytes(dir.getParent().getFullPathName()));
-      statusList.add(status);
+
     }
-    return statusList.toArray(
-        new SnapshotStatus[statusList.size()]);
+    return statuses;
   }
 
   /**
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/AdminHelper.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/AdminHelper.java
@@ -43,7 +43,7 @@ public class AdminHelper {
   static final int MAX_LINE_WIDTH = 80;
   static final String HELP_COMMAND_NAME = "-help";
 
-  static DistributedFileSystem getDFS(Configuration conf)
+  public static DistributedFileSystem getDFS(Configuration conf)
       throws IOException {
     FileSystem fs = FileSystem.get(conf);
     return checkAndGetDFS(fs, conf);
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/snapshot/LsSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/snapshot/LsSnapshot.java
@@ -17,16 +17,13 @@
  */
 package org.apache.hadoop.hdfs.tools.snapshot;
 
-import java.io.IOException;
 
 import org.apache.hadoop.classification.InterfaceAudience;
-import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.conf.Configured;
-import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hdfs.DistributedFileSystem;
 import org.apache.hadoop.hdfs.protocol.SnapshotStatus;
-import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus;
+import org.apache.hadoop.hdfs.tools.AdminHelper;
 import org.apache.hadoop.util.Tool;
 import org.apache.hadoop.util.ToolRunner;
 
@@ -43,26 +40,18 @@ public int run(String[] argv) throws Exception {
         "\tGet the list of snapshots for a snapshottable directory.\n";
 
     if(argv.length != 1) {
+      System.err.println("Invalid no of arguments");
       System.err.println("Usage: \n" + description);
       return 1;
     }
-
-    FileSystem fs = FileSystem.get(getConf());
-    if (! (fs instanceof DistributedFileSystem)) {
-      System.err.println(
-          "lsSnapshot can only be used in DistributedFileSystem");
-      return 1;
-    }
-    DistributedFileSystem dfs = (DistributedFileSystem) fs;
     Path snapshotRoot = new Path(argv[0]);
-
     try {
+      DistributedFileSystem dfs = AdminHelper.getDFS(getConf());
       SnapshotStatus[] stats = dfs.getSnapshotListing(snapshotRoot);
       SnapshotStatus.print(stats, System.out);
-    } catch (IOException e) {
+    } catch (Exception e) {
       String[] content = e.getLocalizedMessage().split("\n");
       System.err.println("lsSnapshot: " + content[0]);
-      e.printStackTrace(System.err);
       return 1;
     }
     return 0;
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestListSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestListSnapshot.java

Original file line number	Diff line number	Diff line change
`@@ -728,7 +728,7 @@ SnapshottableDirectoryStatus[] getSnapshottableDirListing()`
`728`	`728`	`throws IOException;`
`729`	`729`
`730`	`730`	`/**`
`731`		`- * Get listing of all the snapshots for a snapshottable directory`
	`731`	`+ * Get listing of all the snapshots for a snapshottable directory.`
`732`	`732`	`*`
`733`	`733`	`* @return Information about all the snapshots for a snapshottable directory`
`734`	`734`	`* @throws IOException If an I/O error occurred`