xBis7 commented on code in PR #5882:
URL: https://github.com/apache/ozone/pull/5882#discussion_r1448515900


##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -1572,6 +1575,102 @@ public void testScmStatsFromNodeReport()
     }
   }
 
+  private List<StorageReportProto> generateStorageReportProto(
+      int volumeCount, UUID dnId, long capacity, long used) {
+    List<StorageReportProto> reports = new ArrayList<>(volumeCount);
+    boolean failed = true;
+    for (int x = 0; x < volumeCount; x++) {
+      long free = capacity - used;
+      String storagePath = testDir.getAbsolutePath() + "/" + dnId;
+      reports.add(HddsTestUtils
+          .createStorageReport(dnId, storagePath, capacity,
+              used, free, null, failed));
+      failed = !failed;
+    }
+    return reports;
+  }
+
+  private static Stream<Arguments> calculateStoragePercentageScenarios() {
+    return Stream.of(
+        Arguments.of(1000, 600, 10, 60),
+        Arguments.of(1000, 1000, 10, 100),
+        Arguments.of(1000, 1001, 10, 100),
+        Arguments.of(0, 0, 0, 0),
+        Arguments.of(1010, 547, 5, 54.16)
+    );
+  }
+
+  @ParameterizedTest
+  @MethodSource("calculateStoragePercentageScenarios")
+  public void testCalculateStoragePercentage(long capacity,
+      long used, int volumeCount, double targetUsedPerc)
+          throws AuthenticationException, IOException {
+    OzoneConfiguration conf = getConf();
+    conf.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 1000,
+        MILLISECONDS);
+    try (SCMNodeManager nodeManager = createNodeManager(conf)) {
+      EventQueue eventQueue = (EventQueue) scm.getEventQueue();
+      LayoutVersionManager versionManager =
+          nodeManager.getLayoutVersionManager();
+      LayoutVersionProto layoutInfo = toLayoutVersionProto(
+          versionManager.getMetadataLayoutVersion(),
+          versionManager.getSoftwareLayoutVersion());
+      DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
+      UUID dnId = dn.getUuid();
+      List<StorageReportProto> reports = volumeCount > 0 ?
+          generateStorageReportProto(volumeCount, dnId, capacity, used) : null;
+      nodeManager.register(dn, reports != null ?
+          HddsTestUtils.createNodeReport(reports, emptyList()) : null, null);
+      nodeManager.processHeartbeat(dn, layoutInfo);
+      eventQueue.processAll(8000L);
+      NodeStateManager nodeStateManager = nodeManager.getNodeStateManager();
+      List<DatanodeInfo> dataNodeInfoList = nodeStateManager.getAllNodes();
+      double usedPerc = nodeManager.calculateStoragePercentage(
+          dataNodeInfoList.get(0));
+      assertEquals(usedPerc, targetUsedPerc);
+    }
+  }
+
+  private static Stream<Arguments> calculateStorageCapacityScenarios() {
+    return Stream.of(
+        Arguments.of(600, 6, 1, "600.0B"),
+        Arguments.of(10000, 1000, 12, "117.2KB"),
+        Arguments.of(100000000, 1000, 12, "1.1GB"),
+        Arguments.of(10000, 1000, 0, "0.0B")
+    );
+  }
+
+  @ParameterizedTest
+  @MethodSource("calculateStorageCapacityScenarios")
+  public void testcalculateStorageCapacity(long capacity,
+      long used, int volumeCount, String totalCapacity)
+          throws AuthenticationException, IOException {
+    OzoneConfiguration conf = getConf();
+    conf.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 1000,
+        MILLISECONDS);
+    try (SCMNodeManager nodeManager = createNodeManager(conf)) {
+      EventQueue eventQueue = (EventQueue) scm.getEventQueue();
+      LayoutVersionManager versionManager =
+          nodeManager.getLayoutVersionManager();
+      LayoutVersionProto layoutInfo = toLayoutVersionProto(
+          versionManager.getMetadataLayoutVersion(),
+          versionManager.getSoftwareLayoutVersion());
+      DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
+      UUID dnId = dn.getUuid();
+      List<StorageReportProto> reports = volumeCount > 0 ?
+          generateStorageReportProto(volumeCount, dnId, capacity, used) : null;
+      nodeManager.register(dn, reports != null ?
+          HddsTestUtils.createNodeReport(reports, emptyList()) : null, null);
+      nodeManager.processHeartbeat(dn, layoutInfo);
+      eventQueue.processAll(8000L);
+      NodeStateManager nodeStateManager = nodeManager.getNodeStateManager();
+      List<DatanodeInfo> dataNodeInfoList = nodeStateManager.getAllNodes();

Review Comment:
   It looks like, from 
   ```
   EventQueue eventQueue = (EventQueue) scm.getEventQueue();
   ...
   ...
   List<DatanodeInfo> dataNodeInfoList = nodeStateManager.getAllNodes();
   ```
   up to this line, the setup is exactly the same for both methods.
   
   You can move this code to a method that returns the `datanodeInfoList` and 
reuse the method.
   
   Another approach would be to combine both tests in one.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -1113,11 +1117,78 @@ public Map<String, Map<String, String>> 
getNodeStatusInfo() {
         map.put(httpsPort.getName().toString(),
                   httpsPort.getValue().toString());
       }
+      String capacity = calculateStorageCapacity(dni);
+      map.put(totalCapacity, capacity);
+      double usedPec = calculateStoragePercentage(dni);
+      map.put(usedSpacePercent, usedPec + "%");
       nodes.put(hostName, map);
     }
     return nodes;
   }
 
+  /**
+   * Calculate the storage capacity of the DataNode node.
+   * @param dni DataNode node that needs to be calculated.
+   * @return
+   */
+  public String calculateStorageCapacity(DatanodeInfo dni) {
+    long capacityByte = 0;
+    List<StorageReportProto> storageReports = dni.getStorageReports();
+    if (storageReports != null && !storageReports.isEmpty()) {
+      for (StorageReportProto storageReport : storageReports) {
+        capacityByte += storageReport.getCapacity();
+      }
+    }
+
+    double ua = capacityByte;
+    String unit = "B";
+    if (ua > 1024) {
+      ua = ua / 1024;
+      unit = "KB";

Review Comment:
   Strings in java, are immutable. That means that when you create a String 
variable, a new object is created and then the variable references that object. 
Every time you assign a new value to the String variable, a new object gets 
created and the variable's reference gets updated. The old object is left 
around until it gets picked up by the garbage collector.
   
   IMO, in this case it's better to do this kind of string manipulation, using 
a `StringBuilder`.
   
   Check these
   
   
https://www.digitalocean.com/community/tutorials/string-vs-stringbuffer-vs-stringbuilder
   
   https://www.baeldung.com/java-string-performance



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to