[GitHub] [hadoop] jojochuang commented on a diff in pull request #4107: HDFS-16521. DFS API to retrieve slow datanodes

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

jojochuang commented on code in PR #4107:
URL: https://github.com/apache/hadoop/pull/4107#discussion_r859381403


##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java:
##########
@@ -4914,6 +4914,33 @@ int getNumberOfDatanodes(DatanodeReportType type) {
     }
   }
 
+  DatanodeInfo[] slowDataNodesReport() throws IOException {
+    String operationName = "slowDataNodesReport";
+    DatanodeInfo[] datanodeInfos;
+    checkSuperuserPrivilege(operationName);

Review Comment:
   does it need to require super user privilege?



##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java:
##########
@@ -433,7 +433,7 @@ static int run(DistributedFileSystem dfs, String[] argv, int idx) throws IOExcep
    */
   private static final String commonUsageSummary =
     "\t[-report [-live] [-dead] [-decommissioning] " +
-    "[-enteringmaintenance] [-inmaintenance]]\n" +
+      "[-enteringmaintenance] [-inmaintenance] [-slownodes]]\n" +

Review Comment:
   The corresponding documentation needs to update when CLI commands are added/updated.



##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java:
##########
@@ -632,6 +638,20 @@ private static void printDataNodeReports(DistributedFileSystem dfs,
     }
   }
 
+  private static void printSlowDataNodeReports(DistributedFileSystem dfs, boolean listNodes,

Review Comment:
   Can you provide a sample output? It would be confusing, I guess. I suspect you would need some kind of header to distinguish from the other data node reports. 



##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java:
##########
@@ -1868,4 +1868,16 @@ BatchedEntries<OpenFileEntry> listOpenFiles(long prevId,
    */
   @AtMostOnce
   void satisfyStoragePolicy(String path) throws IOException;
+
+  /**
+   * Get report on all of the slow Datanodes. Slow running datanodes are identified based on
+   * the Outlier detection algorithm, if slow peer tracking is enabled for the DFS cluster.
+   *
+   * @return Datanode report for slow running datanodes.
+   * @throws IOException If an I/O error occurs.
+   */
+  @Idempotent
+  @ReadOnly
+  DatanodeInfo[] getSlowDatanodeReport() throws IOException;

Review Comment:
   I just want to check with every one that it is okay to have an array of objects as the return value.
   I think it's fine but just want to check with every one, because once we decide the the interface it can't be changed later.



##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java:
##########
@@ -433,7 +433,7 @@ static int run(DistributedFileSystem dfs, String[] argv, int idx) throws IOExcep
    */
   private static final String commonUsageSummary =
     "\t[-report [-live] [-dead] [-decommissioning] " +
-    "[-enteringmaintenance] [-inmaintenance]]\n" +
+      "[-enteringmaintenance] [-inmaintenance] [-slownodes]]\n" +

Review Comment:
   In fact it would appear confusion to HDFS administrators. These subcommands are meant to filter the DNs in these states, and "slownodes" is not a defined DataNode state.



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

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