HDDS-1422. Exception during DataNode shutdown. Contributed by Arpit A… (#725)

* HDDS-1422. Exception during DataNode shutdown. Contributed by Arpit Agarwal.

Change-Id: I6db6bdd19839a45e5341ed7e745cd38f68af8378

* Suppress spurious findbugs warning.

* Remove log file that got committed in error

Author: arp7

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 @@ private VolumeInfo(Builder b) throws IOException {
 
   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() {

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.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<GetSpaceUsed> 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 @@ long getAvailable() throws IOException {
   }
 
   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 @@ void saveScmUsed() {
    * 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);
   }
 }

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java

@@ -29,14 +29,12 @@
 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 void testShutdown() throws Exception {
     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();
   }
 }

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java

@@ -35,7 +35,6 @@
 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 void testShutdown() throws Exception {
 
     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();
     }
   }