diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java index 0d698982dfa..31f83ec8dab 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java @@ -39,7 +39,8 @@ public final class VolumeInfo { private final StorageType storageType; // Space usage calculator - private VolumeUsage usage; + private final VolumeUsage usage; + // Capacity configured. This is useful when we want to // limit the visible capacity for tests. If negative, then we just // query from the filesystem. @@ -97,36 +98,21 @@ public final class VolumeInfo { public long getCapacity() throws IOException { if (configuredCapacity < 0) { - if (usage == null) { - throw new IOException("Volume Usage thread is not running. This error" + - " is usually seen during DataNode shutdown."); - } return usage.getCapacity(); } return configuredCapacity; } public long getAvailable() throws IOException { - if (usage == null) { - throw new IOException("Volume Usage thread is not running. This error " + - "is usually seen during DataNode shutdown."); - } return usage.getAvailable(); } public long getScmUsed() throws IOException { - if (usage == null) { - throw new IOException("Volume Usage thread is not running. This error " + - "is usually seen during DataNode shutdown."); - } return usage.getScmUsed(); } protected void shutdownUsageThread() { - if (usage != null) { - usage.shutdown(); - } - usage = null; + usage.shutdown(); } public String getRootDir() { diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java index 2c7563e0859..693bcb50cc5 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java @@ -19,6 +19,7 @@ package org.apache.hadoop.ozone.container.common.volume; import com.google.common.annotations.VisibleForTesting; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CachingGetSpaceUsed; import org.apache.hadoop.fs.DF; @@ -35,6 +36,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; import java.util.Scanner; +import java.util.concurrent.atomic.AtomicReference; /** * Class that wraps the space df of the Datanode Volumes used by SCM @@ -46,7 +48,8 @@ public class VolumeUsage { private final File rootDir; private final DF df; private final File scmUsedFile; - private GetSpaceUsed scmUsage; + private AtomicReference scmUsage; + private boolean shutdownComplete; private static final String DU_CACHE_FILE = "scmUsed"; private volatile boolean scmUsedSaved = false; @@ -65,10 +68,11 @@ public class VolumeUsage { void startScmUsageThread(Configuration conf) throws IOException { // get SCM specific df - this.scmUsage = new CachingGetSpaceUsed.Builder().setPath(rootDir) - .setConf(conf) - .setInitialUsed(loadScmUsed()) - .build(); + scmUsage = new AtomicReference<>( + new CachingGetSpaceUsed.Builder().setPath(rootDir) + .setConf(conf) + .setInitialUsed(loadScmUsed()) + .build()); } long getCapacity() { @@ -89,14 +93,18 @@ public class VolumeUsage { } long getScmUsed() throws IOException{ - return scmUsage.getUsed(); + return scmUsage.get().getUsed(); } - public void shutdown() { - saveScmUsed(); + public synchronized void shutdown() { + if (!shutdownComplete) { + saveScmUsed(); - if (scmUsage instanceof CachingGetSpaceUsed) { - IOUtils.cleanupWithLogger(null, ((CachingGetSpaceUsed) scmUsage)); + if (scmUsage.get() instanceof CachingGetSpaceUsed) { + IOUtils.cleanupWithLogger( + null, ((CachingGetSpaceUsed) scmUsage.get())); + } + shutdownComplete = true; } } @@ -175,7 +183,11 @@ public class VolumeUsage { * Only for testing. Do not use otherwise. */ @VisibleForTesting + @SuppressFBWarnings( + value = "IS2_INCONSISTENT_SYNC", + justification = "scmUsage is an AtomicReference. No additional " + + "synchronization is needed.") public void setScmUsageForTesting(GetSpaceUsed scmUsageForTest) { - this.scmUsage = scmUsageForTest; + scmUsage.set(scmUsageForTest); } } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java index 0e58e69864c..fb2f29b6a13 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java @@ -29,14 +29,12 @@ import org.junit.rules.TemporaryFolder; import org.mockito.Mockito; import java.io.File; -import java.io.IOException; import java.util.Properties; import java.util.UUID; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; /** * Unit tests for {@link HddsVolume}. @@ -134,15 +132,8 @@ public class TestHddsVolume { assertTrue("scmUsed cache file should be saved on shutdown", scmUsedFile.exists()); - try { - // Volume.getAvailable() should fail with IOException - // as usage thread is shutdown. - volume.getAvailable(); - fail("HddsVolume#shutdown test failed"); - } catch (Exception ex) { - assertTrue(ex instanceof IOException); - assertTrue(ex.getMessage().contains( - "Volume Usage thread is not running.")); - } + // Volume.getAvailable() should succeed even when usage thread + // is shutdown. + volume.getAvailable(); } } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java index c50ec783817..f97ad4e873f 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java @@ -35,7 +35,6 @@ import static org.apache.hadoop.ozone.container.common.volume.HddsVolume import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import org.junit.After; import org.junit.Assert; @@ -213,18 +212,10 @@ public class TestVolumeSet { volumeSet.shutdown(); - // Verify that the volumes are shutdown and the volumeUsage is set to null. + // Verify that volume usage can be queried during shutdown. for (HddsVolume volume : volumesList) { - Assert.assertNull(volume.getVolumeInfo().getUsageForTesting()); - try { - // getAvailable() should throw null pointer exception as usage is null. - volume.getAvailable(); - fail("Volume shutdown failed."); - } catch (IOException ex) { - // Do Nothing. Exception is expected. - assertTrue(ex.getMessage().contains( - "Volume Usage thread is not running.")); - } + Assert.assertNotNull(volume.getVolumeInfo().getUsageForTesting()); + volume.getAvailable(); } }