errose28 commented on code in PR #3727:
URL: https://github.com/apache/ozone/pull/3727#discussion_r961181431
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerMetadataScanner.java:
##########
@@ -24,128 +24,46 @@
import java.io.IOException;
import java.util.Iterator;
-import java.util.concurrent.TimeUnit;
/**
* This class is responsible to perform metadata verification of the
* containers.
*/
-public class ContainerMetadataScanner extends Thread {
+public class ContainerMetadataScanner extends AbstractContainerScanner {
public static final Logger LOG =
LoggerFactory.getLogger(ContainerMetadataScanner.class);
- private final ContainerController controller;
- private final long metadataScanInterval;
private final ContainerMetadataScrubberMetrics metrics;
- /**
- * True if the thread is stopping.<p/>
- * Protected by this object's lock.
- */
- private boolean stopping = false;
+ private final ContainerController controller;
public ContainerMetadataScanner(ContainerScrubberConfiguration conf,
ContainerController controller) {
+ super(conf.getMetadataScanInterval(),
+ ContainerMetadataScrubberMetrics.create());
this.controller = controller;
- this.metadataScanInterval = conf.getMetadataScanInterval();
- this.metrics = ContainerMetadataScrubberMetrics.create();
+ this.metrics = (ContainerMetadataScrubberMetrics) super.getMetrics();
setName("ContainerMetadataScanner");
setDaemon(true);
}
@Override
- public void run() {
- /*
- * the outer daemon loop exits on shutdown()
- */
- LOG.info("Background ContainerMetadataScanner starting up");
- try {
- while (!stopping) {
- runIteration();
- if (!stopping) {
- metrics.resetNumUnhealthyContainers();
- metrics.resetNumContainersScanned();
- }
- }
- } catch (Exception e) {
- LOG.error("{} exiting because of exception ", this, e);
- } finally {
- if (metrics != null) {
- metrics.unregister();
- }
- }
- }
-
- @VisibleForTesting
- void runIteration() {
- long start = System.nanoTime();
- Iterator<Container<?>> containerIt = controller.getContainers();
- while (!stopping && containerIt.hasNext()) {
- Container container = containerIt.next();
- try {
- scrub(container);
- } catch (IOException e) {
- LOG.info("Unexpected error while scrubbing container {}",
- container.getContainerData().getContainerID());
- } finally {
- metrics.incNumContainersScanned();
- }
- }
- long interval = System.nanoTime() - start;
- if (!stopping) {
- metrics.incNumScanIterations();
- LOG.info("Completed an iteration of container metadata scrubber in" +
- " {} minutes." +
- " Number of iterations (since the data-node restart) : {}" +
- ", Number of containers scanned in this iteration : {}" +
- ", Number of unhealthy containers found in this iteration : {}",
- TimeUnit.NANOSECONDS.toMinutes(interval),
- metrics.getNumScanIterations(),
- metrics.getNumContainersScanned(),
- metrics.getNumUnHealthyContainers());
- long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(interval);
- long remainingSleep = metadataScanInterval - elapsedMillis;
- if (remainingSleep > 0) {
- try {
- Thread.sleep(remainingSleep);
- } catch (InterruptedException e) {
- LOG.info("Background ContainerMetadataScanner interrupted." +
- " Going to exit");
- // Restore the interruption flag and the internal `stopping`
- // variable to prevent the next iteration thus stopping the thread
- interrupt();
- this.stopping = true;
- }
- }
- }
+ public Iterator<Container<?>> getContainerIterator() {
+ return controller.getContainers();
}
@VisibleForTesting
- public void scrub(Container container) throws IOException {
+ @Override
+ public void scanContainer(Container container) throws IOException {
+ metrics.incNumContainersScanned();
Review Comment:
nit. Move this after the if statement since that is when the container has
finished being scanned. Same for the data scanner.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerDataScanner.java:
##########
@@ -32,11 +26,16 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
+import java.time.Instant;
+import java.util.Iterator;
+import java.util.Optional;
+
/**
* VolumeScanner scans a single volume. Each VolumeScanner has its own thread.
* <p>They are all managed by the DataNode's BlockScanner.
Review Comment:
This comment can be updated. This class is no longer the VolumeScanner, and
I think this second line is obsolete. I don't know of a BlockScanner or
equivalent class on the datanode.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerMetadataScanner.java:
##########
@@ -24,128 +24,46 @@
import java.io.IOException;
import java.util.Iterator;
-import java.util.concurrent.TimeUnit;
/**
* This class is responsible to perform metadata verification of the
* containers.
*/
-public class ContainerMetadataScanner extends Thread {
+public class ContainerMetadataScanner extends AbstractContainerScanner {
public static final Logger LOG =
LoggerFactory.getLogger(ContainerMetadataScanner.class);
- private final ContainerController controller;
- private final long metadataScanInterval;
private final ContainerMetadataScrubberMetrics metrics;
- /**
- * True if the thread is stopping.<p/>
- * Protected by this object's lock.
- */
- private boolean stopping = false;
+ private final ContainerController controller;
public ContainerMetadataScanner(ContainerScrubberConfiguration conf,
ContainerController controller) {
+ super(conf.getMetadataScanInterval(),
+ ContainerMetadataScrubberMetrics.create());
this.controller = controller;
- this.metadataScanInterval = conf.getMetadataScanInterval();
- this.metrics = ContainerMetadataScrubberMetrics.create();
+ this.metrics = (ContainerMetadataScrubberMetrics) super.getMetrics();
setName("ContainerMetadataScanner");
setDaemon(true);
Review Comment:
Looks like this setDaemon call can be removed here since the parent is
already setting it. Also a minor thing but the thread name can be a parameter
to the parent constructor who would call setName instead of each child doing it.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerMetadataScanner.java:
##########
@@ -24,128 +24,46 @@
import java.io.IOException;
import java.util.Iterator;
-import java.util.concurrent.TimeUnit;
/**
* This class is responsible to perform metadata verification of the
* containers.
Review Comment:
Let's add to the javadoc that only one thread will be responsible for doing
metadata scans across all volumes.
--
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]
