aswinshakil commented on code in PR #8296:
URL: https://github.com/apache/ozone/pull/8296#discussion_r2048570937
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OnDemandContainerDataScanner.java:
##########
@@ -178,35 +161,27 @@ private static void logScanStart(ContainerData
containerData) {
}
}
- private static void logScanCompleted(
+ private void logScanCompleted(
ContainerData containerData, Instant timestamp) {
LOG.debug("Completed scan of container {} at {}",
containerData.getContainerID(), timestamp);
}
- public static OnDemandScannerMetrics getMetrics() {
- return instance.metrics;
+ public OnDemandScannerMetrics getMetrics() {
+ return metrics;
}
@VisibleForTesting
- public static DataTransferThrottler getThrottler() {
- return instance.throttler;
+ public DataTransferThrottler getThrottler() {
Review Comment:
Looks like this is not being used. If it's not being used in the follow-up
patch, we can remove it.
Same for line 180 `getCanceler()`
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerSet.java:
##########
@@ -284,6 +285,28 @@ public void
testListContainerFromFirstKey(ContainerLayoutVersion layout)
assertContainerIds(FIRST_ID, count, result);
}
+ @ContainerLayoutTestInfo.ContainerTest
+ public void testContainerScanHandler(ContainerLayoutVersion layout) throws
Exception {
+ setLayoutVersion(layout);
+ ContainerSet containerSet = createContainerSet();
+ // Scan when no handler is registered should not throw an exception.
+ containerSet.scanContainer(FIRST_ID);
+
+ // Scan of non-existent container should not throw exception or trigger
the handler.
+ containerSet.scanContainer(FIRST_ID - 1);
+ AtomicLong invocationCount = new AtomicLong();
+ containerSet.registerContainerScanHandler(c -> {
+ // If the handler was incorrectly triggered for a non-existent
container, this assert would fail.
+ assertEquals(c.getContainerData().getContainerID(), FIRST_ID);
Review Comment:
```suggestion
assertEquals(FIRST_ID, c.getContainerData().getContainerID());
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java:
##########
@@ -96,14 +99,13 @@ public void testUnscannedContainerIsScanned() throws
Exception {
@AfterEach
public void tearDown() {
- OnDemandContainerDataScanner.shutdown();
+ onDemandScanner.shutdown();
}
@Test
public void testScanTimestampUpdated() throws Exception {
- OnDemandContainerDataScanner.init(conf, controller);
Optional<Future<?>> scanFuture =
- OnDemandContainerDataScanner.scanContainer(healthy);
+ onDemandScanner.scanContainer(healthy);
Review Comment:
These can be moved to line 107. There are other line similar these below, We
can move them to the line above.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
@@ -432,7 +433,8 @@ private void
initOnDemandContainerScanner(ContainerScannerConfiguration c) {
"so the on-demand container data scanner will not start.");
return;
}
- OnDemandContainerDataScanner.init(c, controller);
+ onDemandScanner = new OnDemandContainerDataScanner(c, controller);
+ containerSet.registerContainerScanHandler(onDemandScanner::scanContainer);
Review Comment:
Can we pass the `OnDemandContainerDataScanner` itself here instead of using
Consumer?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java:
##########
@@ -161,19 +156,18 @@ public void testSameContainerQueuedMultipleTimes() throws
Exception {
@Test
@Override
public void testScannerMetrics() throws Exception {
- OnDemandContainerDataScanner.init(conf, controller);
ArrayList<Optional<Future<?>>> resultFutureList = Lists.newArrayList();
- resultFutureList.add(OnDemandContainerDataScanner.scanContainer(
+ resultFutureList.add(onDemandScanner.scanContainer(
Review Comment:
Same as above.
--
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]
