HDFS-11267. Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage. (Manoj Govindassamy via lei)

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java

@@ -125,7 +125,7 @@ public abstract class Storage extends StorageInfo {
   }
 
   protected List<StorageDirectory> storageDirs =
-      new CopyOnWriteArrayList<StorageDirectory>();
+      new CopyOnWriteArrayList<>();
   
   private class DirIterator implements Iterator<StorageDirectory> {
     final StorageDirType dirType;
@@ -939,6 +939,10 @@ public abstract class Storage extends StorageInfo {
     return storageDirs.size();
   }
 
+  public List<StorageDirectory> getStorageDirs() {
+    return storageDirs;
+  }
+
   public StorageDirectory getStorageDir(int idx) {
     return storageDirs.get(idx);
   }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java

@@ -295,7 +295,7 @@ public class BlockPoolSliceStorage extends Storage {
   void remove(File absPathToRemove) {
     Preconditions.checkArgument(absPathToRemove.isAbsolute());
     LOG.info("Removing block level storage: " + absPathToRemove);
-    for (Iterator<StorageDirectory> it = this.storageDirs.iterator();
+    for (Iterator<StorageDirectory> it = getStorageDirs().iterator();
          it.hasNext(); ) {
       StorageDirectory sd = it.next();
       if (sd.getRoot().getAbsoluteFile().equals(absPathToRemove)) {
@@ -788,7 +788,7 @@ public class BlockPoolSliceStorage extends Storage {
    */
   public void clearTrash() {
     final List<File> trashRoots = new ArrayList<>();
-    for (StorageDirectory sd : storageDirs) {
+    for (StorageDirectory sd : getStorageDirs()) {
       File trashRoot = getTrashRootDir(sd);
       if (trashRoot.exists() && sd.getPreviousDir().exists()) {
         LOG.error("Trash and PreviousDir shouldn't both exist for storage "
@@ -826,7 +826,7 @@ public class BlockPoolSliceStorage extends Storage {
   /** trash is enabled if at least one storage directory contains trash root */
   @VisibleForTesting
   public boolean trashEnabled() {
-    for (StorageDirectory sd : storageDirs) {
+    for (StorageDirectory sd : getStorageDirs()) {
       if (getTrashRootDir(sd).exists()) {
         return true;
       }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java

@@ -181,11 +181,11 @@ public class DataStorage extends Storage {
   }
 
   public void setRollingUpgradeMarker(String bpid) throws IOException {
-    getBPStorage(bpid).setRollingUpgradeMarkers(storageDirs);
+    getBPStorage(bpid).setRollingUpgradeMarkers(getStorageDirs());
   }
 
   public void clearRollingUpgradeMarker(String bpid) throws IOException {
-    getBPStorage(bpid).clearRollingUpgradeMarkers(storageDirs);
+    getBPStorage(bpid).clearRollingUpgradeMarkers(getStorageDirs());
   }
 
   /**
@@ -493,7 +493,7 @@ public class DataStorage extends Storage {
     }
 
     StringBuilder errorMsgBuilder = new StringBuilder();
-    for (Iterator<StorageDirectory> it = this.storageDirs.iterator();
+    for (Iterator<StorageDirectory> it = getStorageDirs().iterator();
          it.hasNext(); ) {
       StorageDirectory sd = it.next();
       StorageLocation sdLocation = sd.getStorageLocation();
@@ -988,7 +988,7 @@ public class DataStorage extends Storage {
     // To handle finalizing a snapshot taken at datanode level while
     // upgrading to federation, if datanode level snapshot previous exists, 
     // then finalize it. Else finalize the corresponding BP.
-    for (StorageDirectory sd : storageDirs) {
+    for (StorageDirectory sd : getStorageDirs()) {
       File prevDir = sd.getPreviousDir();
       if (prevDir.exists()) {
         // data node level storage finalize

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java

@@ -172,8 +172,6 @@ public class NNStorage extends Storage implements Closeable,
       throws IOException {
     super(NodeType.NAME_NODE);
 
-    storageDirs = new CopyOnWriteArrayList<>();
-    
     // this may modify the editsDirs, so copy before passing in
     setStorageDirectories(imageDirs, 
                           Lists.newArrayList(editsDirs),
@@ -212,7 +210,7 @@ public class NNStorage extends Storage implements Closeable,
   @Override // Closeable
   public void close() throws IOException {
     unlockAll();
-    storageDirs.clear();
+    getStorageDirs().clear();
   }
 
   /**
@@ -295,7 +293,7 @@ public class NNStorage extends Storage implements Closeable,
                                           Collection<URI> fsEditsDirs,
                                           Collection<URI> sharedEditsDirs)
       throws IOException {
-    this.storageDirs.clear();
+    getStorageDirs().clear();
     this.removedStorageDirs.clear();
 
    // Add all name dirs with appropriate NameNodeDirType
@@ -873,7 +871,7 @@ public class NNStorage extends Storage implements Closeable,
                +  sd.getRoot().getPath(), e);
     }
 
-    if (this.storageDirs.remove(sd)) {
+    if (getStorageDirs().remove(sd)) {
       this.removedStorageDirs.add(sd);
     }
     
@@ -928,7 +926,7 @@ public class NNStorage extends Storage implements Closeable,
     // getCanonicalPath may need to call stat() or readlink() and it's likely
     // those calls would fail due to the same underlying IO problem.
     String absPath = f.getAbsolutePath();
-    for (StorageDirectory sd : storageDirs) {
+    for (StorageDirectory sd : getStorageDirs()) {
       String dirPath = sd.getRoot().getAbsolutePath();
       if (!dirPath.endsWith(File.separator)) {
         dirPath += File.separator;
@@ -1140,14 +1138,14 @@ public class NNStorage extends Storage implements Closeable,
   @Override
   public void writeAll() throws IOException {
     this.layoutVersion = getServiceLayoutVersion();
-    for (StorageDirectory sd : storageDirs) {
+    for (StorageDirectory sd : getStorageDirs()) {
       try {
         writeProperties(sd);
       } catch (Exception e) {
         LOG.warn("Error during write properties to the VERSION file to " +
             sd.toString(), e);
         reportErrorsOnDirectory(sd);
-        if (storageDirs.isEmpty()) {
+        if (getStorageDirs().isEmpty()) {
           throw new IOException("All the storage failed while writing " +
               "properties to VERSION file");
         }

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/StorageAdapter.java

@@ -33,7 +33,7 @@ public abstract class StorageAdapter {
       Storage s, int idx) {
 
     StorageDirectory dir = Mockito.spy(s.getStorageDir(idx));
-    s.storageDirs.set(idx, dir);
+    s.getStorageDirs().set(idx, dir);
     return dir;
   }
 }

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java

@@ -50,7 +50,7 @@ public class TestBlockPoolSliceStorage {
                               String clusterId) {
       super(namespaceID, bpID, cTime, clusterId);
       addStorageDir(new StorageDirectory(new File("/tmp/dontcare/" + bpID)));
-      assertThat(storageDirs.size(), is(1));
+      assertThat(getStorageDirs().size(), is(1));
     }
   }