Copilot commented on code in PR #10988:
URL: https://github.com/apache/ozone/pull/10988#discussion_r3754139402
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java:
##########
@@ -407,20 +408,44 @@ public boolean hasEnoughVolumes() {
public StorageLocationReport[] getStorageReport() {
this.readLock();
try {
- StorageLocationReport[] reports = new
StorageLocationReport[volumeMap.size() + failedVolumeMap.size()];
- int counter = 0;
- for (StorageVolume volume : volumeMap.values()) {
- reports[counter++] = volume.getReport();
- }
- for (StorageVolume volume : failedVolumeMap.values()) {
- reports[counter++] = volume.getReport();
- }
- return reports;
+ return buildStorageReport();
} finally {
Review Comment:
`getStorageReport()` is used on the node-report path (eg
`OzoneContainer#getNodeReport()`), so it’s performance-sensitive. The refactor
to `buildStorageReport()` now allocates an `ArrayList` plus an array on every
call; previously it only allocated the result array. Consider keeping the
original pre-sized array implementation for the locking `getStorageReport()`
method, and reserve the `ArrayList` approach for the lock-free snapshot where
sizes can change concurrently.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java:
##########
@@ -30,7 +30,7 @@
/**
* Node-level storage totals for a DataNode, aggregated over its HDDS data
volumes only
- * ({@code VolumeType.DATA_VOLUME}) via {@link
MutableVolumeSet#getStorageReport()}.
+ * ({@code VolumeType.DATA_VOLUME}) via {@link
MutableVolumeSet#getStorageReportSnapshot()}.
* This is the same scope as the {@code storageReport} entries produced by
* {@code OzoneContainer.getNodeReport()}; meta and DB volumes are excluded.
* Registered as {@code
Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics}.
Review Comment:
The class Javadoc says this source is registered as
`Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics`, but in mini-cluster
mode the record/source name is now suffixed with the datanode UUID
(`DatanodeStorageMetrics-<uuid>`). Please update the Javadoc to reflect the
conditional naming so operators/test readers aren’t misled when inspecting
JMX/Prometheus output from mini-clusters.
--
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]
