Copilot commented on code in PR #10312:
URL: https://github.com/apache/ozone/pull/10312#discussion_r3271059000
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -206,13 +220,14 @@ private Map<String, Object>
toJson(DatanodeDiskBalancerInfoProto report) {
Map<String, Object> vm = new LinkedHashMap<>();
vm.put("storageId", v.getStorageId());
vm.put("storagePath", v.hasStoragePath() ? v.getStoragePath() : "-");
- vm.put("totalCapacity", v.hasTotalCapacity() ?
StringUtils.byteDesc(v.getTotalCapacity()) : "-");
- vm.put("usedSpace", v.hasUsedSpace() ?
StringUtils.byteDesc(v.getUsedSpace()) : "-");
+ vm.put("ozoneCapacity", v.hasOzoneCapacity() ?
StringUtils.byteDesc(v.getOzoneCapacity()) : "-");
+ vm.put("ozoneAvailable", v.hasOzoneAvailable() ?
StringUtils.byteDesc(v.getOzoneAvailable()) : "-");
+ vm.put("ozoneUsed", v.hasOzoneUsedSpace() ?
StringUtils.byteDesc(v.getOzoneUsedSpace()) : "-");
vm.put("containerPreAllocatedSpace",
StringUtils.byteDesc(v.getCommittedBytes()));
vm.put("effectiveUsedSpace", v.hasEffectiveUsedSpace() ?
StringUtils.byteDesc(v.getEffectiveUsedSpace()) : "-");
- vm.put("utilization", v.getUtilization());
- vm.put("volumeDensity", Math.abs(v.getUtilization() - ideal));
+ vm.put("utilization", formatPercent(v.getUtilization()));
+ vm.put("volumeDensity", formatPercent(Math.abs(v.getUtilization() -
ideal)));
vols.add(vm);
Review Comment:
The JSON output changes the types/units of several fields: `volumeDensity`
(top-level) and per-volume `utilization`/`volumeDensity` were previously
numeric ratios, but are now percentage strings with a `%` suffix. This is a
backwards-incompatible change for any tooling that parses `--json` output as
numbers. Prefer keeping JSON fields numeric (either as ratios or numeric
percents) and, if needed, add separate formatted string fields for display.
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -586,11 +586,12 @@ enum DiskBalancerRunningStatus {
message VolumeReportProto {
optional string storageId = 1;
optional string storagePath = 2;
- optional uint64 totalCapacity = 3;
- optional uint64 usedSpace = 4;
- optional uint64 committedBytes = 5;
- optional uint64 effectiveUsedSpace = 6;
- optional double utilization = 7;
+ optional uint64 ozoneCapacity = 3;
+ optional uint64 ozoneAvailable = 4;
+ optional uint64 ozoneUsedSpace = 5;
+ optional uint64 committedBytes = 6;
+ optional uint64 effectiveUsedSpace = 7;
+ optional double utilization = 8;
Review Comment:
The protobuf field tags in `VolumeReportProto` have been reassigned (e.g.,
tag 4 used to be `usedSpace` but is now `ozoneAvailable`, and subsequent fields
are renumbered). Reusing/renumbering protobuf tags breaks wire compatibility:
older clients will deserialize `ozoneAvailable` into `usedSpace`, and will not
see the real used space at all. Keep existing tag numbers for existing
semantics, deprecate old field names if needed, and add new fields (eg
`ozoneAvailable`) with new unused tag numbers; consider reserving the old
names/tags to prevent accidental reuse.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -122,27 +126,29 @@ private String
generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
contentList.add(header.toString());
if (p.getVolumeInfoCount() > 0 && p.hasIdealUsage()) {
- formatBuilder.append("%-45s %-40s %15s %15s %30s %20s %15s %15s%n");
+ formatBuilder.append("%-45s %-40s %15s %15s %15s %30s %20s %5s %5s%n");
contentList.add("StorageID");
contentList.add("StoragePath");
- contentList.add("TotalCapacity");
- contentList.add("UsedSpace");
+ contentList.add("OzoneCapacity");
+ contentList.add("OzoneAvailable");
+ contentList.add("OzoneUsed");
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");
+ formatBuilder.append("%-45s %-40s %15s %15s %15s %30s %15s %6s
%6s%n");
contentList.add(v.hasStorageId() ? v.getStorageId() : "-");
Review Comment:
The format strings for the volume table use different column widths for the
Utilization/VolumeDensity columns between the header (`%5s %5s`) and rows (`%6s
%6s`). This can cause misaligned output, and widths of 5–6 chars are also too
small for values like `100.00%`. Consider using consistent widths for
header/rows and sizing them to accommodate the longest percentage string.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -160,12 +166,15 @@ private String
generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
.append(" IdealUsage +/- Threshold are considered balanced.%n")
.append(" - VolumeDensity: Deviation of a particular volume's
utilization from IdealUsage.%n")
.append(" - Utilization: Ratio of actual used space to capacity (0-1)
for a particular volume.%n")
Review Comment:
The help text says Utilization/IdealUsage are "ratio (0-1)", but the output
is now formatted as a percentage with a `%` suffix (via `formatPercent`).
Please update the explanatory note to match the actual units shown (either
describe them as percentages, or change the formatting to continue showing
ratios).
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerCommands.java:
##########
@@ -155,11 +157,10 @@
@Command(
name = "diskbalancer",
- description = "DiskBalancer specific operations. It is disabled by
default." +
- " To enable it, set 'hdds.datanode.disk.balancer.enabled' as true",
+ description = "DiskBalancer specific operations to ensure even disk
utilisation." +
Review Comment:
CLI help text in this module predominantly uses "utilization" (US spelling),
but this new top-level description uses "utilisation". Consider standardizing
the spelling across the CLI help output for consistency.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerReportSubcommand.java:
##########
@@ -110,10 +114,10 @@ private String
generateReport(List<DatanodeDiskBalancerInfoProto> protos) {
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("IdealUsage: ").append(formatPercent(idealUsage))
.append(" | Threshold: ").append(threshold).append('%')
Review Comment:
`Threshold` is appended using the raw `double` value (`append(threshold)`),
while other displayed ratios are rounded to 2 decimals. If the goal is
consistent 2-decimal output, format `threshold` as well (e.g., `%.2f%%`) so
values like `10.0`/`0.0100000003` don’t show inconsistent precision.
--
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]
