[GitHub] [ozone] siddhantsangwan commented on a change in pull request #1982: HDDS-4872. Upgrade UsageInfoSubcommand with options to show most and least used datanodes.

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

siddhantsangwan commented on a change in pull request #1982:
URL: https://github.com/apache/ozone/pull/1982#discussion_r589241505



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -678,13 +680,111 @@ public boolean getReplicationManagerStatus() {
     long used = stat.getScmUsed().get();
     long remaining = stat.getRemaining().get();
 
-    HddsProtos.DatanodeUsageInfo info = HddsProtos.DatanodeUsageInfo
-        .newBuilder()
+    return HddsProtos.DatanodeUsageInfo.newBuilder()
         .setCapacity(capacity)
         .setUsed(used)
         .setRemaining(remaining)
+        .setNode(node.toProto(node.getCurrentVersion()))
         .build();
-    return info;
+  }
+
+  /**
+   * Get information of the most or least used datanodes.
+   *
+   * @param mostUsed true if most used, false if least used
+   * @param count Integer number of nodes to get info for
+   * @return List of DatanodeUsageInfo. Each element contains usage info
+   * such as capacity, SCMUsed, and remaining space.
+   * @throws IOException
+   */
+  @Override
+  public List<HddsProtos.DatanodeUsageInfo> getDatanodeUsageInfo(
+      boolean mostUsed, int count) throws IOException {
+
+    // check admin authorisation
+    String remoteUser = getRpcRemoteUsername();
+    try {
+      getScm().checkAdminAccess(remoteUser);
+    } catch (IOException e) {
+      LOG.error("Authorisation failed", e);
+      throw e;
+    }
+
+    PriorityQueue<DatanodeDetails> nodes = null;
+    if (mostUsed) {
+      nodes = getMostUsedDatanodes(count);
+    } else {
+      nodes = getLeastUsedDatanodes(count);
+    }
+
+    List<HddsProtos.DatanodeUsageInfo> infoList = new ArrayList<>();
+    for (DatanodeDetails node : nodes) {
+      infoList.add(getUsageInfoFromDatanodeDetails(node));

Review comment:
       When writing `getMostUsedDatanodes` and `getLeastUsedDatanodes`, I intended their purpose to be returning the most used and least used datanodes respectively so that methods higher up in the method call can do whatever they want with that information.
   
   So do you suggest adding another parameter `NodeManager` to `getUsageInfoFromDatanodeDetails` and checking that parameter for null? If it's null, I can call `scm.getNodeManager` there.
   
   OR 
   
   Return a list of DatanodeUsageInfo from `getMostUsedDatanodes` (can be refactored to `getMostOrLeastUsedDatanodes`)?




----------------------------------------------------------------
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org