Copilot commented on code in PR #9969:
URL: https://github.com/apache/ozone/pull/9969#discussion_r3007380623
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -121,20 +176,47 @@ protected String getActionName() {
/**
* Create a JSON result map for a report.
- *
+ *
* @param report the DiskBalancer report proto
* @return JSON result map
*/
- private Map<String, Object> createReportResult(
- HddsProtos.DatanodeDiskBalancerInfoProto report) {
+ private Map<String, Object> toJson(DatanodeDiskBalancerInfoProto report) {
Map<String, Object> result = new LinkedHashMap<>();
- // Format datanode string with hostname and IP address
- String formattedDatanode = DiskBalancerSubCommandUtil.getDatanodeHostAndIp(
- report.getNode());
- result.put("datanode", formattedDatanode);
+ result.put("datanode",
DiskBalancerSubCommandUtil.getDatanodeHostAndIp(report.getNode()));
result.put("action", "report");
result.put("status", "success");
result.put("volumeDensity", report.getCurrentVolumeDensitySum());
+
+ if (report.hasIdealUsage() && report.hasDiskBalancerConf()
+ && report.getDiskBalancerConf().hasThreshold()) {
+ double idealUsage = report.getIdealUsage();
+ double threshold = report.getDiskBalancerConf().getThreshold();
+ double lt = idealUsage - threshold / 100.0;
+ double ut = idealUsage + threshold / 100.0;
+ result.put("idealUsage", String.format("%.8f", idealUsage));
+ result.put("threshold %", report.getDiskBalancerConf().getThreshold());
+ result.put("thresholdRange", String.format("(%.08f, %.08f)", lt, ut));
Review Comment:
The JSON keys/types introduced here are inconsistent with existing
DiskBalancer JSON output (eg status uses numeric `threshold`). Using a key with
a space/percent sign (`"threshold %"`) and emitting `idealUsage` as a formatted
string makes the JSON harder to consume programmatically. Prefer stable
identifier-style keys (eg `thresholdPercent`) and keep numeric values as
numbers.
```suggestion
result.put("idealUsage", idealUsage);
result.put("thresholdPercent", threshold);
Map<String, Double> thresholdRange = new LinkedHashMap<>();
thresholdRange.put("lower", lt);
thresholdRange.put("upper", ut);
result.put("thresholdRange", thresholdRange);
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -121,20 +176,47 @@ protected String getActionName() {
/**
* Create a JSON result map for a report.
- *
+ *
* @param report the DiskBalancer report proto
* @return JSON result map
*/
- private Map<String, Object> createReportResult(
- HddsProtos.DatanodeDiskBalancerInfoProto report) {
+ private Map<String, Object> toJson(DatanodeDiskBalancerInfoProto report) {
Map<String, Object> result = new LinkedHashMap<>();
- // Format datanode string with hostname and IP address
- String formattedDatanode = DiskBalancerSubCommandUtil.getDatanodeHostAndIp(
- report.getNode());
- result.put("datanode", formattedDatanode);
+ result.put("datanode",
DiskBalancerSubCommandUtil.getDatanodeHostAndIp(report.getNode()));
result.put("action", "report");
result.put("status", "success");
result.put("volumeDensity", report.getCurrentVolumeDensitySum());
+
+ if (report.hasIdealUsage() && report.hasDiskBalancerConf()
+ && report.getDiskBalancerConf().hasThreshold()) {
+ double idealUsage = report.getIdealUsage();
+ double threshold = report.getDiskBalancerConf().getThreshold();
+ double lt = idealUsage - threshold / 100.0;
+ double ut = idealUsage + threshold / 100.0;
+ result.put("idealUsage", String.format("%.8f", idealUsage));
+ result.put("threshold %", report.getDiskBalancerConf().getThreshold());
+ result.put("thresholdRange", String.format("(%.08f, %.08f)", lt, ut));
Review Comment:
As in the text output, the computed thresholdRange bounds can go outside [0,
1] and produce invalid ranges in JSON. Clamp `lt`/`ut` before formatting so the
reported range reflects valid utilization ratios.
```suggestion
double ltClamped = Math.max(0.0, Math.min(1.0, lt));
double utClamped = Math.max(0.0, Math.min(1.0, ut));
result.put("idealUsage", String.format("%.8f", idealUsage));
result.put("threshold %", report.getDiskBalancerConf().getThreshold());
result.put("thresholdRange",
String.format("(%.08f, %.08f)", ltClamped, utClamped));
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -82,36 +83,90 @@ protected void displayResults(List<String> successNodes,
List<String> failedNode
// Display consolidated report for successful nodes
if (!successNodes.isEmpty() && !reports.isEmpty()) {
- List<HddsProtos.DatanodeDiskBalancerInfoProto> reportList =
- new ArrayList<>(reports.values());
+ List<DatanodeDiskBalancerInfoProto> reportList = new
ArrayList<>(reports.values());
System.out.println(generateReport(reportList));
}
}
private String generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
- // Sort by volume density in descending order (highest imbalance first)
- List<DatanodeDiskBalancerInfoProto> sortedProtos = new ArrayList<>(protos);
- sortedProtos.sort((a, b) ->
+ protos.sort((a, b) ->
Double.compare(b.getCurrentVolumeDensitySum(),
a.getCurrentVolumeDensitySum()));
- StringBuilder formatBuilder = new StringBuilder("Report result:%n" +
- "%-60s %s%n");
-
+ StringBuilder formatBuilder = new StringBuilder("Report result:%n");
List<String> contentList = new ArrayList<>();
- contentList.add("Datanode");
- contentList.add("VolumeDensity");
-
- for (DatanodeDiskBalancerInfoProto proto : sortedProtos) {
- formatBuilder.append("%-60s %s%n");
- // Format datanode string with hostname and IP address
- String formattedDatanode =
DiskBalancerSubCommandUtil.getDatanodeHostAndIp(
- proto.getNode());
- contentList.add(formattedDatanode);
- contentList.add(String.valueOf(proto.getCurrentVolumeDensitySum()));
+
+ for (int i = 0; i < protos.size(); i++) {
+ DatanodeDiskBalancerInfoProto p = protos.get(i);
+ String dn = DiskBalancerSubCommandUtil.getDatanodeHostAndIp(p.getNode());
+
+ StringBuilder header = new StringBuilder();
+ header.append("Datanode: ").append(dn).append('\n');
+ header.append("Aggregate VolumeDataDensity: ").
+ append(p.getCurrentVolumeDensitySum()).append('\n');
+
+ if (p.hasIdealUsage() && p.hasDiskBalancerConf()
+ && p.getDiskBalancerConf().hasThreshold()) {
+ double idealUsage = p.getIdealUsage();
+ double threshold = p.getDiskBalancerConf().getThreshold();
+ double lt = idealUsage - threshold / 100.0;
+ double ut = idealUsage + threshold / 100.0;
+ header.append("IdealUsage: ").append(String.format("%.8f",
idealUsage));
+ header.append(" | Threshold: ").append(threshold).append('%');
+ header.append(" | ThresholdRange: (").append(String.format("%.8f",
lt));
+ header.append(", ").append(String.format("%.8f",
ut)).append(')').append('\n').append('\n');
+ header.append("Volume Details -:").append('\n');
+ }
+ formatBuilder.append("%s%n");
+ contentList.add(header.toString());
+
+ if (p.getVolumeInfoCount() > 0 && p.hasIdealUsage()) {
+ formatBuilder.append("%-45s %-40s %15s %15s %30s %20s %15s %15s%n");
+ contentList.add("StorageID");
+ contentList.add("StoragePath");
+ contentList.add("TotalCapacity");
+ contentList.add("UsedSpace");
+ contentList.add("Container Pre-AllocatedSpace");
+ contentList.add("EffectiveUsedSpace");
+ contentList.add("Utilization");
+ contentList.add("VolumeDensity");
+
+ double ideal = p.getIdealUsage();
+ for (VolumeReportProto v : p.getVolumeInfoList()) {
+ formatBuilder.append("%-45s %-40s %15s %15s %30s %20s %15s %15s%n");
+ contentList.add(v.getStorageId() != null ? v.getStorageId() : "-");
Review Comment:
`v.getStorageId()` is never null for protobuf-generated accessors (it
defaults to an empty string when unset). The current null-check won’t emit "-"
for missing storageId. Use `v.hasStorageId()` (and/or
`!v.getStorageId().isEmpty()`) to decide when to print a placeholder.
```suggestion
contentList.add(v.hasStorageId() ? v.getStorageId() : "-");
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -82,36 +83,90 @@ protected void displayResults(List<String> successNodes,
List<String> failedNode
// Display consolidated report for successful nodes
if (!successNodes.isEmpty() && !reports.isEmpty()) {
- List<HddsProtos.DatanodeDiskBalancerInfoProto> reportList =
- new ArrayList<>(reports.values());
+ List<DatanodeDiskBalancerInfoProto> reportList = new
ArrayList<>(reports.values());
System.out.println(generateReport(reportList));
}
}
private String generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
- // Sort by volume density in descending order (highest imbalance first)
- List<DatanodeDiskBalancerInfoProto> sortedProtos = new ArrayList<>(protos);
- sortedProtos.sort((a, b) ->
+ protos.sort((a, b) ->
Double.compare(b.getCurrentVolumeDensitySum(),
a.getCurrentVolumeDensitySum()));
- StringBuilder formatBuilder = new StringBuilder("Report result:%n" +
- "%-60s %s%n");
-
+ StringBuilder formatBuilder = new StringBuilder("Report result:%n");
List<String> contentList = new ArrayList<>();
- contentList.add("Datanode");
- contentList.add("VolumeDensity");
-
- for (DatanodeDiskBalancerInfoProto proto : sortedProtos) {
- formatBuilder.append("%-60s %s%n");
- // Format datanode string with hostname and IP address
- String formattedDatanode =
DiskBalancerSubCommandUtil.getDatanodeHostAndIp(
- proto.getNode());
- contentList.add(formattedDatanode);
- contentList.add(String.valueOf(proto.getCurrentVolumeDensitySum()));
+
+ for (int i = 0; i < protos.size(); i++) {
+ DatanodeDiskBalancerInfoProto p = protos.get(i);
+ String dn = DiskBalancerSubCommandUtil.getDatanodeHostAndIp(p.getNode());
+
+ StringBuilder header = new StringBuilder();
+ header.append("Datanode: ").append(dn).append('\n');
+ header.append("Aggregate VolumeDataDensity: ").
+ append(p.getCurrentVolumeDensitySum()).append('\n');
+
+ if (p.hasIdealUsage() && p.hasDiskBalancerConf()
+ && p.getDiskBalancerConf().hasThreshold()) {
+ double idealUsage = p.getIdealUsage();
+ double threshold = p.getDiskBalancerConf().getThreshold();
+ double lt = idealUsage - threshold / 100.0;
+ double ut = idealUsage + threshold / 100.0;
Review Comment:
ThresholdRange is computed as idealUsage +/- threshold/100 without clamping
to [0, 1]. If idealUsage is near 0 or 1 (or threshold is large), this can print
negative or >1 ranges, which is misleading to users. Clamp the computed
lower/upper bounds before formatting.
```suggestion
double margin = threshold / 100.0;
double lt = Math.max(0.0, idealUsage - margin);
double ut = Math.min(1.0, idealUsage + margin);
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -27,22 +27,23 @@
import java.util.concurrent.ConcurrentHashMap;
import org.apache.hadoop.hdds.cli.HddsVersionProvider;
import org.apache.hadoop.hdds.protocol.DiskBalancerProtocol;
-import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import
org.apache.hadoop.hdds.protocol.proto.HddsProtos.DatanodeDiskBalancerInfoProto;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos.VolumeReportProto;
+import org.apache.hadoop.util.StringUtils;
import picocli.CommandLine.Command;
/**
* Handler to get disk balancer report.
*/
@Command(
name = "report",
- description = "Get DiskBalancer volume density report",
+ description = "Get DiskBalancer volume density report and per volume info
from dns.",
Review Comment:
The command description says "from dns" which is ambiguous (looks like
Domain Name System). Consider changing to "from DNs" or "from datanodes" for
clarity, and drop the trailing period to match other subcommand descriptions.
```suggestion
description = "Get DiskBalancer volume density report and per volume
info from datanodes",
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
@@ -685,9 +688,42 @@ public DiskBalancerInfo getDiskBalancerInfo() {
bytesToMove = calculateBytesToMove(volumeUsages);
}
- return new DiskBalancerInfo(operationalState, threshold, bandwidthInMB,
+ DiskBalancerInfo info = new DiskBalancerInfo(operationalState, threshold,
bandwidthInMB,
parallelThread, stopAfterDiskEven, version, metrics.getSuccessCount(),
metrics.getFailureCount(), bytesToMove, metrics.getSuccessBytes(),
volumeDataDensity);
+ info.setIdealUsage(getIdealUsage(volumeUsages));
+ info.setVolumeInfo(buildVolumeReportProto(volumeUsages));
+ return info;
+ }
+
+ /**
+ * Build a list of VolumeReportProto from a list of VolumeFixedUsage.
+ * VolumeReportProto consists of information like StorageID,
+ * volume utilization and committed bytes to the client.
+ *
+ * @param volumeSet snapshot of VolumeFixedUsage which contains the usage
information of each volume
+ * @return a list of VolumeReportProto which will be sent to clients for
reporting volume status
+ */
+ public static List<VolumeReportProto>
buildVolumeReportProto(List<VolumeFixedUsage> volumeSet) {
+ if (volumeSet == null || volumeSet.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<VolumeReportProto> result = new ArrayList<>();
+ for (VolumeFixedUsage v : volumeSet) {
+ HddsVolume volume = v.getVolume();
+ String path = volume.getStorageDir() != null ?
volume.getStorageDir().getPath() : "";
+ result.add(VolumeReportProto.newBuilder()
+ .setStorageId(volume.getStorageID())
+ .setStoragePath(path)
+ .setTotalCapacity(v.getUsage().getCapacity())
+ .setUsedSpace(v.getUsage().getUsedSpace())
+ .setCommittedBytes(volume.getCommittedBytes())
+ .setEffectiveUsedSpace(v.getEffectiveUsed())
+ .setUtilization(v.getUtilization())
+ .build());
Review Comment:
When `volume.getStorageDir()` is null, `path` is set to an empty string and
then written into the proto, which makes `hasStoragePath()` true and causes the
CLI to print an empty value instead of the intended placeholder. Avoid setting
`storagePath` when it’s unknown (or use a meaningful placeholder consistently).
```suggestion
VolumeReportProto.Builder builder = VolumeReportProto.newBuilder()
.setStorageId(volume.getStorageID())
.setTotalCapacity(v.getUsage().getCapacity())
.setUsedSpace(v.getUsage().getUsedSpace())
.setCommittedBytes(volume.getCommittedBytes())
.setEffectiveUsedSpace(v.getEffectiveUsed())
.setUtilization(v.getUtilization());
if (volume.getStorageDir() != null) {
builder.setStoragePath(volume.getStorageDir().getPath());
}
result.add(builder.build());
```
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestDiskBalancerSubCommands.java:
##########
@@ -754,9 +763,56 @@ private DatanodeDiskBalancerInfoProto
generateRandomReportProto(String hostname)
.build())
.build();
+ DiskBalancerConfigurationProto configProto = createConfigProto(threshold,
100L, 5, true);
+
+ long committed1 = (random.nextLong() & Long.MAX_VALUE) % (1024L * 1024 *
1024);
+ long committed2 = (random.nextLong() & Long.MAX_VALUE) % (1024L * 1024 *
1024);
+ long capacity1 = 500L * 1024 * 1024 * 1024 + random.nextInt(1024 * 1024);
+ long capacity2 = 600L * 1024 * 1024 * 1024 + random.nextInt(1024 * 1024);
+ double util1 = idealUsage + random.nextDouble() * 0.1;
+ double util2 = idealUsage - random.nextDouble() * 0.1;
+ long used1 = (long) (capacity1 * util1);
+ long used2 = (long) (capacity2 * util2);
+ long effective1 = used1 + committed1;
+ long effective2 = used2 + committed2;
+ String path1 = "/data/hdds-" + hostname + "-1";
+ String path2 = "/data/hdds-" + hostname + "-2";
+ VolumeReportProto vol1 = VolumeReportProto.newBuilder()
+ .setStorageId("DISK-" + hostname + "-vol1")
+ .setStoragePath(path1)
+ .setUtilization(idealUsage + random.nextDouble() * 0.1)
Review Comment:
In this test helper, `used1` is computed using `util1`, but the proto’s
`vol1` utilization is generated independently (`idealUsage +
random.nextDouble() * 0.1`). This can make the generated report internally
inconsistent (utilization doesn’t match usedSpace/capacity). Use `util1` when
setting `vol1` utilization so the fields stay consistent.
```suggestion
.setUtilization(util1)
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -82,36 +83,90 @@ protected void displayResults(List<String> successNodes,
List<String> failedNode
// Display consolidated report for successful nodes
if (!successNodes.isEmpty() && !reports.isEmpty()) {
- List<HddsProtos.DatanodeDiskBalancerInfoProto> reportList =
- new ArrayList<>(reports.values());
+ List<DatanodeDiskBalancerInfoProto> reportList = new
ArrayList<>(reports.values());
System.out.println(generateReport(reportList));
}
}
private String generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
- // Sort by volume density in descending order (highest imbalance first)
- List<DatanodeDiskBalancerInfoProto> sortedProtos = new ArrayList<>(protos);
- sortedProtos.sort((a, b) ->
+ protos.sort((a, b) ->
Double.compare(b.getCurrentVolumeDensitySum(),
a.getCurrentVolumeDensitySum()));
- StringBuilder formatBuilder = new StringBuilder("Report result:%n" +
- "%-60s %s%n");
-
+ StringBuilder formatBuilder = new StringBuilder("Report result:%n");
List<String> contentList = new ArrayList<>();
- contentList.add("Datanode");
- contentList.add("VolumeDensity");
-
- for (DatanodeDiskBalancerInfoProto proto : sortedProtos) {
- formatBuilder.append("%-60s %s%n");
- // Format datanode string with hostname and IP address
- String formattedDatanode =
DiskBalancerSubCommandUtil.getDatanodeHostAndIp(
- proto.getNode());
- contentList.add(formattedDatanode);
- contentList.add(String.valueOf(proto.getCurrentVolumeDensitySum()));
+
+ for (int i = 0; i < protos.size(); i++) {
+ DatanodeDiskBalancerInfoProto p = protos.get(i);
+ String dn = DiskBalancerSubCommandUtil.getDatanodeHostAndIp(p.getNode());
+
+ StringBuilder header = new StringBuilder();
+ header.append("Datanode: ").append(dn).append('\n');
+ header.append("Aggregate VolumeDataDensity: ").
+ append(p.getCurrentVolumeDensitySum()).append('\n');
+
+ if (p.hasIdealUsage() && p.hasDiskBalancerConf()
+ && p.getDiskBalancerConf().hasThreshold()) {
+ double idealUsage = p.getIdealUsage();
+ double threshold = p.getDiskBalancerConf().getThreshold();
+ double lt = idealUsage - threshold / 100.0;
+ double ut = idealUsage + threshold / 100.0;
+ header.append("IdealUsage: ").append(String.format("%.8f",
idealUsage));
+ header.append(" | Threshold: ").append(threshold).append('%');
+ header.append(" | ThresholdRange: (").append(String.format("%.8f",
lt));
+ header.append(", ").append(String.format("%.8f",
ut)).append(')').append('\n').append('\n');
+ header.append("Volume Details -:").append('\n');
Review Comment:
The header label "Volume Details -:" is awkward/typo-like in user-facing
output. Consider renaming to "Volume Details:" (or similar) for readability.
```suggestion
header.append("Volume Details:").append('\n');
```
--
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]
