errose28 commented on code in PR #7335:
URL: https://github.com/apache/ozone/pull/7335#discussion_r1811515370
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OnDemandContainerDataScanner.java:
##########
@@ -128,6 +128,7 @@ private static void performOnDemandScan(Container<?>
container) {
return;
}
+ long startTime = System.nanoTime();
Review Comment:
Let's use `Instant.now().toEpochMillis()` to be consistent with the rest of
the code. I think `millis` will be precise enough.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/AbstractContainerScannerMetrics.java:
##########
@@ -42,6 +43,8 @@ public abstract class AbstractContainerScannerMetrics {
private MutableGaugeInt numUnHealthyContainers;
@Metric("number of iterations of scanner completed since the restart")
private MutableCounterInt numScanIterations;
+ @Metric("total time that the container scanned has been running, in
milliseconds.")
+ private MutableCounterLong totalRunTimes;
Review Comment:
```suggestion
@Metric("total time that the container scanner has been running, in
milliseconds.")
private MutableCounterLong totalRunTime;
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java:
##########
@@ -104,6 +104,12 @@ public void testScannerMetrics() {
assertEquals(1, metrics.getNumUnHealthyContainers());
}
+ @Test
+ public void testTotalRunTimes() {
+ scanner.runIteration();
+ assertTrue(scanner.getMetrics().getTotalRunTimes() > 0);
Review Comment:
Also, can we add this to the existing metrics test instead? Currently this
is the only metric tested outside of that method.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java:
##########
@@ -104,6 +104,12 @@ public void testScannerMetrics() {
assertEquals(1, metrics.getNumUnHealthyContainers());
}
+ @Test
+ public void testTotalRunTimes() {
+ scanner.runIteration();
+ assertTrue(scanner.getMetrics().getTotalRunTimes() > 0);
Review Comment:
Same for the metadata test.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java:
##########
@@ -104,6 +104,12 @@ public void testScannerMetrics() {
assertEquals(1, metrics.getNumUnHealthyContainers());
}
+ @Test
+ public void testTotalRunTimes() {
+ scanner.runIteration();
+ assertTrue(scanner.getMetrics().getTotalRunTimes() > 0);
Review Comment:
We can make this a little more robust by modifying one of the container
mocks to have a small sleep during its scan. The mocked containers are directly
accessible from this test, so you could make it so that when one of the
container's `scanData` methods is called it will sleep for 500ms. Then this
test would check that the total run time is > 500ms. We can then run the scan
again and check that it goes up to > 1 second. These are just example numbers,
we could probably use lower values.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/AbstractContainerScannerMetrics.java:
##########
@@ -42,6 +43,8 @@ public abstract class AbstractContainerScannerMetrics {
private MutableGaugeInt numUnHealthyContainers;
@Metric("number of iterations of scanner completed since the restart")
private MutableCounterInt numScanIterations;
+ @Metric("total time that the container scanned has been running, in
milliseconds.")
+ private MutableCounterLong totalRunTimes;
Review Comment:
We can rename the getters and setters too.
--
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]
