Copilot commented on code in PR #10304:
URL: https://github.com/apache/ozone/pull/10304#discussion_r3308983202
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfoMetrics.java:
##########
@@ -51,14 +51,20 @@ public class VolumeInfoMetrics implements MetricsSource {
Interns.info("OzoneUsed", "Ozone used space");
private static final MetricsInfo RESERVED =
Interns.info("Reserved", "Reserved Space");
- private static final MetricsInfo TOTAL_CAPACITY =
- Interns.info("TotalCapacity", "Ozone capacity + reserved space");
private static final MetricsInfo FS_CAPACITY =
Interns.info("FilesystemCapacity", "Filesystem capacity as reported by
the local filesystem");
private static final MetricsInfo FS_AVAILABLE =
Interns.info("FilesystemAvailable", "Filesystem available space as
reported by the local filesystem");
private static final MetricsInfo FS_USED =
Interns.info("FilesystemUsed", "Filesystem used space
(FilesystemCapacity - FilesystemAvailable)");
Review Comment:
Removing the `TotalCapacity` JMX metric is a backward-incompatible change
for existing dashboards/alerts. Even if it is redundant with
`FilesystemCapacity`, consider keeping it as a deprecated alias (eg, emit
`TotalCapacity = FilesystemCapacity`) for at least one release cycle and/or
clearly marking it deprecated in the metric description so consumers have time
to migrate.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeInfoMetrics.java:
##########
@@ -72,8 +74,11 @@ void testVolumeInfoMetricsExposeOzoneAndFilesystemGauges() {
assertThat(findMetric(all, "OzoneUsed")).isEqualTo(100L);
assertThat(findMetric(all, "FilesystemCapacity")).isEqualTo(2000L);
- assertThat(findMetric(all, "FilesystemAvailable")).isEqualTo(1500L);
- assertThat(findMetric(all, "FilesystemUsed")).isEqualTo(500L);
+ assertThat(findMetric(all, "FilesystemAvailable")).isEqualTo(1100L);
+ assertThat(findMetric(all, "FilesystemUsed")).isEqualTo(900L); //
FilesystemCapacity - FilesystemAvailable
+
+ assertThat(findMetric(all, "MinFreeSpace")).isEqualTo(20L);
+ assertThat(findMetric(all, "NonOzoneUsed")).isEqualTo(400L);
} finally {
Review Comment:
In this test, the mocked `volumeUsage.getCurrentUsage(...)` returns
`OzoneUsed=100` while `realUsage()` returns a `Fixed(..., used=500)`. The newly
asserted `NonOzoneUsed` is derived from `realUsage` (via
`VolumeUsage.getOtherUsed`), so the test currently contradicts the metric
contract described in `VolumeInfoMetrics` (NonOzoneUsed = FilesystemUsed -
OzoneUsed) and isn’t representative of real `VolumeUsage` behavior (where
`getCurrentUsage(real)` preserves `real.getUsedSpace()`). Please align the
mocks so `usage.getUsedSpace()` matches `fsUsage.getUsedSpace()` (and update
the expected Ozone*/NonOzoneUsed values accordingly), or alternatively adjust
the inputs so the formula holds in the test data.
--
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]
