adoroszlai commented on code in PR #10564:
URL: https://github.com/apache/ozone/pull/10564#discussion_r3449924364
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
+ } finally {
+ volume.shutdown();
+ }
+ }
+
+ @Test
+ public void testFailVolumeShouldUnregisterVolumeInfoMetricsSource() throws
Exception {
Review Comment:
This test case about metrics belongs to `TestVolumeInfoMetrics`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -203,6 +203,9 @@ public void failVolume() {
if (volumeIOStats != null) {
volumeIOStats.unregister();
}
+ if (volumeInfoMetrics != null) {
+ volumeInfoMetrics.unregister();
Review Comment:
`volumeInfoMetrics` is created in constructor even for failed volume. If we
unregister it for healthy volumes that fail later, behavior of initially failed
vs. later failed volume will be different.
Also, I think metrics provides info about basic information that even a
failed volume has. Do we lose that by unregistering metrics?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
Review Comment:
Please add this assertion in existing `testFailedVolumeSpace` test case. No
need for each assertion about failed volume to be in a separate one.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
+ } finally {
+ volume.shutdown();
+ }
+ }
+
+ @Test
+ public void testFailVolumeShouldUnregisterVolumeInfoMetricsSource() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ try {
+ volume = volumeBuilder.build();
+
assertNotNull(DefaultMetricsSystem.instance().getSource(metricsSourceName));
+ volume.failVolume();
+ assertNull(DefaultMetricsSystem.instance().getSource(metricsSourceName),
+ "VolumeInfoMetrics source should be unregistered when a volume
transitions to FAILED");
+ } finally {
+ if (volume != null) {
+ volume.shutdown();
+ }
+ metricsSystem.stop();
+ metricsSystem.shutdown();
+ }
+ }
+
+ @Test
+ public void
testConcurrentMetricsSamplingAfterFailVolumeDoesNotEmitCommittedNpe() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-concurrent-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ LogCapturer methodMetricLogs = LogCapturer.captureLogs(
+
LoggerFactory.getLogger("org.apache.hadoop.metrics2.lib.MethodMetric"));
+ methodMetricLogs.clearOutput();
+
+ AtomicBoolean running = new AtomicBoolean(true);
+ AtomicLong publishCount = new AtomicLong(0);
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ Future<?> publisher = executor.submit(() -> {
+ while (running.get()) {
+ metricsSystem.publishMetricsNow();
+ publishCount.incrementAndGet();
+ }
+ });
+
+ try {
+ volume = volumeBuilder.build();
+
+ // First, prove this test can hit the real MethodMetric -> getCommitted
NPE path.
+ // We force committedBytes to null and invoke metrics snapshot directly.
+ Field committedBytes =
HddsVolume.class.getDeclaredField("committedBytes");
+ committedBytes.setAccessible(true);
+ committedBytes.set(volume, null);
+ assertNull(committedBytes.get(volume),
+ "Failed to force committedBytes to null; reproducer is invalid");
+ volume.getVolumeInfoStats().getMetrics(new MetricsCollectorImpl(), true);
+ String directSnapshotOutput = methodMetricLogs.getOutput();
+ assertTrue(directSnapshotOutput.contains("Error invoking method
getCommitted")
+ || directSnapshotOutput.contains("because
\"this.committedBytes\" is null"),
+ "Reproducer sanity check failed: expected MethodMetric/getCommitted
null-path log");
+ methodMetricLogs.clearOutput();
+ BooleanSupplier initialPublishReady = () -> publishCount.get() >= 20;
+ GenericTestUtils.waitFor(initialPublishReady, 50, 5000);
+
+ assertTrue(publishCount.get() >= 20);
Review Comment:
Please use `assertThat(<int>).isGreaterThanOrEqual(...)`.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
+ } finally {
+ volume.shutdown();
+ }
+ }
+
+ @Test
+ public void testFailVolumeShouldUnregisterVolumeInfoMetricsSource() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ try {
+ volume = volumeBuilder.build();
+
assertNotNull(DefaultMetricsSystem.instance().getSource(metricsSourceName));
+ volume.failVolume();
+ assertNull(DefaultMetricsSystem.instance().getSource(metricsSourceName),
+ "VolumeInfoMetrics source should be unregistered when a volume
transitions to FAILED");
+ } finally {
+ if (volume != null) {
+ volume.shutdown();
+ }
+ metricsSystem.stop();
+ metricsSystem.shutdown();
+ }
+ }
+
+ @Test
+ public void
testConcurrentMetricsSamplingAfterFailVolumeDoesNotEmitCommittedNpe() throws
Exception {
Review Comment:
Please remove this test case. It tests a fictional scenario, reflectively
setting a `final`, non-`null` field to `null` just to prove something.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
+ } finally {
+ volume.shutdown();
+ }
+ }
+
+ @Test
+ public void testFailVolumeShouldUnregisterVolumeInfoMetricsSource() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ try {
+ volume = volumeBuilder.build();
+
assertNotNull(DefaultMetricsSystem.instance().getSource(metricsSourceName));
+ volume.failVolume();
+ assertNull(DefaultMetricsSystem.instance().getSource(metricsSourceName),
+ "VolumeInfoMetrics source should be unregistered when a volume
transitions to FAILED");
+ } finally {
+ if (volume != null) {
+ volume.shutdown();
+ }
+ metricsSystem.stop();
+ metricsSystem.shutdown();
+ }
+ }
+
+ @Test
+ public void
testConcurrentMetricsSamplingAfterFailVolumeDoesNotEmitCommittedNpe() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-concurrent-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ LogCapturer methodMetricLogs = LogCapturer.captureLogs(
+
LoggerFactory.getLogger("org.apache.hadoop.metrics2.lib.MethodMetric"));
+ methodMetricLogs.clearOutput();
+
+ AtomicBoolean running = new AtomicBoolean(true);
+ AtomicLong publishCount = new AtomicLong(0);
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ Future<?> publisher = executor.submit(() -> {
+ while (running.get()) {
+ metricsSystem.publishMetricsNow();
+ publishCount.incrementAndGet();
+ }
+ });
+
+ try {
+ volume = volumeBuilder.build();
+
+ // First, prove this test can hit the real MethodMetric -> getCommitted
NPE path.
+ // We force committedBytes to null and invoke metrics snapshot directly.
+ Field committedBytes =
HddsVolume.class.getDeclaredField("committedBytes");
+ committedBytes.setAccessible(true);
+ committedBytes.set(volume, null);
+ assertNull(committedBytes.get(volume),
+ "Failed to force committedBytes to null; reproducer is invalid");
+ volume.getVolumeInfoStats().getMetrics(new MetricsCollectorImpl(), true);
+ String directSnapshotOutput = methodMetricLogs.getOutput();
+ assertTrue(directSnapshotOutput.contains("Error invoking method
getCommitted")
+ || directSnapshotOutput.contains("because
\"this.committedBytes\" is null"),
+ "Reproducer sanity check failed: expected MethodMetric/getCommitted
null-path log");
+ methodMetricLogs.clearOutput();
+ BooleanSupplier initialPublishReady = () -> publishCount.get() >= 20;
+ GenericTestUtils.waitFor(initialPublishReady, 50, 5000);
+
+ assertTrue(publishCount.get() >= 20);
+ volume.failVolume();
+ assertNull(DefaultMetricsSystem.instance().getSource(metricsSourceName),
+ "VolumeInfoMetrics source should be removed after failVolume()");
+
+ long baseline = publishCount.get();
+ BooleanSupplier steadyStatePublishReady = () -> publishCount.get() >=
baseline + 50;
+ GenericTestUtils.waitFor(steadyStatePublishReady, 50, 5000);
+
+ String output = methodMetricLogs.getOutput();
+ assertFalse(output.contains("Error invoking method getCommitted"),
+ "MethodMetric should not invoke getCommitted on failed/unregistered
volume source");
+ assertFalse(output.contains("because \"this.committedBytes\" is null"),
+ "MethodMetric should not hit committedBytes null after failed volume
is unregistered");
Review Comment:
Please use `assertThat(<string>).doesNotContain(...)`.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -531,6 +543,109 @@ public void testFailedVolumeSpace() throws IOException {
}
}
+ @Test
+ public void testCommittedBytesIsInitializedForFailedVolume() throws
Exception {
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+ try {
+ assertEquals(0L, volume.getCommittedBytes(),
+ "committedBytes must be initialized for failed volume instances");
+ } finally {
+ volume.shutdown();
+ }
+ }
+
+ @Test
+ public void testFailVolumeShouldUnregisterVolumeInfoMetricsSource() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ try {
+ volume = volumeBuilder.build();
+
assertNotNull(DefaultMetricsSystem.instance().getSource(metricsSourceName));
+ volume.failVolume();
+ assertNull(DefaultMetricsSystem.instance().getSource(metricsSourceName),
+ "VolumeInfoMetrics source should be unregistered when a volume
transitions to FAILED");
+ } finally {
+ if (volume != null) {
+ volume.shutdown();
+ }
+ metricsSystem.stop();
+ metricsSystem.shutdown();
+ }
+ }
+
+ @Test
+ public void
testConcurrentMetricsSamplingAfterFailVolumeDoesNotEmitCommittedNpe() throws
Exception {
+ DefaultMetricsSystem.shutdown();
+ MetricsSystem metricsSystem = DefaultMetricsSystem.instance();
+ metricsSystem.init("test-hdds-volume-concurrent-metrics");
+ String metricsSourceName = VolumeInfoMetrics.class.getSimpleName() + '-' +
folder.toString();
+ HddsVolume volume = null;
+
+ LogCapturer methodMetricLogs = LogCapturer.captureLogs(
+
LoggerFactory.getLogger("org.apache.hadoop.metrics2.lib.MethodMetric"));
+ methodMetricLogs.clearOutput();
+
+ AtomicBoolean running = new AtomicBoolean(true);
+ AtomicLong publishCount = new AtomicLong(0);
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ Future<?> publisher = executor.submit(() -> {
+ while (running.get()) {
+ metricsSystem.publishMetricsNow();
+ publishCount.incrementAndGet();
+ }
+ });
+
+ try {
+ volume = volumeBuilder.build();
+
+ // First, prove this test can hit the real MethodMetric -> getCommitted
NPE path.
+ // We force committedBytes to null and invoke metrics snapshot directly.
+ Field committedBytes =
HddsVolume.class.getDeclaredField("committedBytes");
+ committedBytes.setAccessible(true);
+ committedBytes.set(volume, null);
+ assertNull(committedBytes.get(volume),
+ "Failed to force committedBytes to null; reproducer is invalid");
+ volume.getVolumeInfoStats().getMetrics(new MetricsCollectorImpl(), true);
+ String directSnapshotOutput = methodMetricLogs.getOutput();
+ assertTrue(directSnapshotOutput.contains("Error invoking method
getCommitted")
+ || directSnapshotOutput.contains("because
\"this.committedBytes\" is null"),
Review Comment:
Please use `assertThat(<string>).contains(...)`, see HDDS-9951.
--
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]
