adoroszlai commented on code in PR #5882:
URL: https://github.com/apache/ozone/pull/5882#discussion_r1457337062
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -1113,11 +1117,94 @@ public Map<String, Map<String, String>>
getNodeStatusInfo() {
map.put(httpsPort.getName().toString(),
httpsPort.getValue().toString());
}
+ String capacity = calculateStorageCapacity(dni);
+ map.put(totalCapacity, capacity);
+ double[] storagePercentage = calculateStoragePercentage(dni);
+ double scmUsedPerc = storagePercentage[0];
+ double nonScmUsedPerc = storagePercentage[1];
+ map.put(usedSpacePercent,
+ "scmUsed: " + scmUsedPerc + "%, nonScmUsed: " + nonScmUsedPerc +
"%");
Review Comment:
I'm not sure `"scmUsed"` and `"nonScmUsed"` are clear enough for users.
Also, the table header already says `"Used Space Percent"`.
How about `"Ozone: ... , other: ..."`?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -1572,6 +1575,67 @@ public void testScmStatsFromNodeReport()
}
}
+ private List<StorageReportProto> generateStorageReportProto(
+ int volumeCount, UUID dnId, long capacity, long used, long remaining) {
+ List<StorageReportProto> reports = new ArrayList<>(volumeCount);
+ boolean failed = true;
+ for (int x = 0; x < volumeCount; x++) {
+ String storagePath = testDir.getAbsolutePath() + "/" + dnId;
+ reports.add(HddsTestUtils
+ .createStorageReport(dnId, storagePath, capacity,
+ used, remaining, null, failed));
+ failed = !failed;
+ }
+ return reports;
+ }
+
+ private static Stream<Arguments> calculateStoragePercentageScenarios() {
+ return Stream.of(
+ Arguments.of(600, 65, 500, 1, "600.0B", 10.83, 5.83),
+ Arguments.of(10000, 1000, 8800, 12, "117.2KB", 10.0, 2.0),
+ Arguments.of(100000000, 1000, 899999, 12, "1.1GB", 0.0, 99.1),
+ Arguments.of(10000, 1000, 0, 0, "0.0B", 0.0, 0.0),
+ Arguments.of(0, 0, 0, 0, "0.0B", 0.0, 0.0),
+ Arguments.of(1010, 547, 400, 5, "4.9KB", 54.16, 6.24)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("calculateStoragePercentageScenarios")
+ public void testCalculateStoragePercentage(long perCapacity,
+ long used, long remaining, int volumeCount, String totalCapacity,
+ double scmUsedPerc, double nonScmUsedPerc)
+ 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, perCapacity,
+ used, remaining) : 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();
+ DatanodeInfo dni = dataNodeInfoList.get(0);
+ String capacityResult = nodeManager.calculateStorageCapacity(dni);
+ assertEquals(capacityResult, totalCapacity);
Review Comment:
Please follow `assertEquals(expected, actual)` argument order.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -1572,6 +1575,67 @@ public void testScmStatsFromNodeReport()
}
}
+ private List<StorageReportProto> generateStorageReportProto(
+ int volumeCount, UUID dnId, long capacity, long used, long remaining) {
+ List<StorageReportProto> reports = new ArrayList<>(volumeCount);
+ boolean failed = true;
+ for (int x = 0; x < volumeCount; x++) {
+ String storagePath = testDir.getAbsolutePath() + "/" + dnId;
+ reports.add(HddsTestUtils
+ .createStorageReport(dnId, storagePath, capacity,
+ used, remaining, null, failed));
+ failed = !failed;
+ }
+ return reports;
+ }
+
+ private static Stream<Arguments> calculateStoragePercentageScenarios() {
+ return Stream.of(
+ Arguments.of(600, 65, 500, 1, "600.0B", 10.83, 5.83),
+ Arguments.of(10000, 1000, 8800, 12, "117.2KB", 10.0, 2.0),
+ Arguments.of(100000000, 1000, 899999, 12, "1.1GB", 0.0, 99.1),
+ Arguments.of(10000, 1000, 0, 0, "0.0B", 0.0, 0.0),
+ Arguments.of(0, 0, 0, 0, "0.0B", 0.0, 0.0),
+ Arguments.of(1010, 547, 400, 5, "4.9KB", 54.16, 6.24)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("calculateStoragePercentageScenarios")
+ public void testCalculateStoragePercentage(long perCapacity,
+ long used, long remaining, int volumeCount, String totalCapacity,
+ double scmUsedPerc, double nonScmUsedPerc)
+ 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, perCapacity,
+ used, remaining) : 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();
+ DatanodeInfo dni = dataNodeInfoList.get(0);
Review Comment:
If `calculate...` methods are changed as suggested, all we need here is:
```
List<StorageReportProto> reports = volumeCount > 0 ?
generateStorageReportProto(volumeCount, dnId, perCapacity,
used, remaining) : null;
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -1113,11 +1117,94 @@ public Map<String, Map<String, String>>
getNodeStatusInfo() {
map.put(httpsPort.getName().toString(),
httpsPort.getValue().toString());
}
+ String capacity = calculateStorageCapacity(dni);
+ map.put(totalCapacity, capacity);
+ double[] storagePercentage = calculateStoragePercentage(dni);
+ double scmUsedPerc = storagePercentage[0];
+ double nonScmUsedPerc = storagePercentage[1];
+ map.put(usedSpacePercent,
+ "scmUsed: " + scmUsedPerc + "%, nonScmUsed: " + nonScmUsedPerc +
"%");
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;
+ StringBuilder unit = new StringBuilder("B");
+ if (ua > 1024) {
+ ua = ua / 1024;
+ unit.replace(0, 1, "KB");
+ }
+ if (ua > 1024) {
+ ua = ua / 1024;
+ unit.replace(0, 2, "MB");
+ }
+ if (ua > 1024) {
+ ua = ua / 1024;
+ unit.replace(0, 2, "GB");
+ }
+ if (ua > 1024) {
+ ua = ua / 1024;
+ unit.replace(0, 2, "TB");
+ }
+
+ DecimalFormat decimalFormat = new DecimalFormat("#0.0");
+ decimalFormat.setRoundingMode(RoundingMode.HALF_UP);
+ double capacity = Double.valueOf(decimalFormat.format(ua));
+ return capacity + unit.toString();
+ }
+
+ /**
+ * Calculate the storage usage percentage of a DataNode node.
+ * @param dni DataNode node that needs to be calculated.
+ * @return
+ */
+ public double[] calculateStoragePercentage(DatanodeInfo dni) {
+ double[] storagePercentage = new double[2];
+ double usedPercentage = 0;
+ double nonUsedPercentage = 0;
+ List<StorageReportProto> storageReports = dni.getStorageReports();
+ if (storageReports != null && !storageReports.isEmpty()) {
+ long capacity = 0;
+ long scmUsed = 0;
+ long remaining = 0;
+ for (StorageReportProto storageReport : storageReports) {
+ capacity += storageReport.getCapacity();
+ scmUsed += storageReport.getScmUsed();
+ remaining += storageReport.getRemaining();
+ }
+ long scmNonUsed = capacity - scmUsed - remaining;
+
+ DecimalFormat decimalFormat = new DecimalFormat("#0.00");
+ decimalFormat.setRoundingMode(RoundingMode.HALF_UP);
+
+ double usedPerc = ((double) scmUsed / capacity) * 100;
+ usedPerc = usedPerc > 100.0 ? 100.0 : usedPerc;
+ double nonUsedPerc = ((double) scmNonUsed / capacity) * 100;
+ nonUsedPerc = nonUsedPerc > 100.0 ? 100.0 : nonUsedPerc;
+ usedPercentage = Double.valueOf(decimalFormat.format(usedPerc));
+ nonUsedPercentage = Double.valueOf(decimalFormat.format(nonUsedPerc));
Review Comment:
Here we format the value, then parse it as double. The caller then simply
concatenates them into the final text being displayed. I think formatting may
be lost this way.
I think `calculateStoragePercentage()` should return the formatted `String`
instead.
If there are no reports, return `"N/A"`, since we have no information.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -1113,11 +1117,94 @@ public Map<String, Map<String, String>>
getNodeStatusInfo() {
map.put(httpsPort.getName().toString(),
httpsPort.getValue().toString());
}
+ String capacity = calculateStorageCapacity(dni);
+ map.put(totalCapacity, capacity);
+ double[] storagePercentage = calculateStoragePercentage(dni);
+ double scmUsedPerc = storagePercentage[0];
+ double nonScmUsedPerc = storagePercentage[1];
+ map.put(usedSpacePercent,
+ "scmUsed: " + scmUsedPerc + "%, nonScmUsed: " + nonScmUsedPerc +
"%");
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) {
Review Comment:
Both new calculate methods can be `static` and take
`List<StorageReportProto>` as parameter instead of `DatanodeInfo`.
This allows the test to be much simpler (see other comment on the test for
details).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -141,6 +143,8 @@ public class SCMNodeManager implements NodeManager {
private final String opeState = "OPSTATE";
private final String comState = "COMSTATE";
private final String lastHeartbeat = "LASTHEARTBEAT";
+ private final String usedSpacePercent = "USEDSPACEPERCENT";
+ private final String totalCapacity = "CAPACITY";
Review Comment:
These should be constants. (The existing ones, too, but those are being
fixed in HDDS-10152.)
--
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]
