HDFS-4500. Refactor snapshot INode methods.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-2802@1446355 13f79535-47bb-0310-9956-ffa450edef68

hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt

@@ -155,3 +155,5 @@ Branch-2802 Snapshot (Unreleased)
   HDFS-4480. Eliminate the file snapshot circular linked list.  (szetszwo)
 
   HDFS-4481. Change fsimage to support snapshot file diffs.  (szetszwo)
+
+  HDFS-4500. Refactor snapshot INode methods.  (szetszwo)

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

@@ -48,17 +48,6 @@ import com.google.common.primitives.SignedBytes;
 public abstract class INode implements Diff.Element<byte[]> {
   public static final Log LOG = LogFactory.getLog(INode.class);
 
-  /** A pair of objects. */
-  public static class Pair<L, R> {
-    public final L left;
-    public final R right;
-
-    public Pair(L left, R right) {
-      this.left = left;
-      this.right = right;
-    }
-  }
-
   /** Wrapper of two counters for namespace consumed and diskspace consumed. */
   static class DirCounts {
     /** namespace count */
@@ -177,12 +166,12 @@ public abstract class INode implements Diff.Element<byte[]> {
     this.permission = that.permission;
   }
   /** Get the {@link PermissionStatus} */
-  public PermissionStatus getPermissionStatus(Snapshot snapshot) {
+  public final PermissionStatus getPermissionStatus(Snapshot snapshot) {
     return new PermissionStatus(getUserName(snapshot), getGroupName(snapshot),
         getFsPermission(snapshot));
   }
   /** The same as getPermissionStatus(null). */
-  public PermissionStatus getPermissionStatus() {
+  public final PermissionStatus getPermissionStatus() {
     return getPermissionStatus(null);
   }
   private INode updatePermissionStatus(PermissionStatusFormat f, long n,
@@ -197,12 +186,16 @@ public abstract class INode implements Diff.Element<byte[]> {
    *          otherwise, get the result from the current inode.
    * @return user name
    */
-  public String getUserName(Snapshot snapshot) {
+  public final String getUserName(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).getUserName();
+    }
+
     int n = (int)PermissionStatusFormat.USER.retrieve(permission);
     return SerialNumberManager.INSTANCE.getUser(n);
   }
   /** The same as getUserName(null). */
-  public String getUserName() {
+  public final String getUserName() {
     return getUserName(null);
   }
   /** Set user */
@@ -216,12 +209,16 @@ public abstract class INode implements Diff.Element<byte[]> {
    *          otherwise, get the result from the current inode.
    * @return group name
    */
-  public String getGroupName(Snapshot snapshot) {
+  public final String getGroupName(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).getGroupName();
+    }
+
     int n = (int)PermissionStatusFormat.GROUP.retrieve(permission);
     return SerialNumberManager.INSTANCE.getGroup(n);
   }
   /** The same as getGroupName(null). */
-  public String getGroupName() {
+  public final String getGroupName() {
     return getGroupName(null);
   }
   /** Set group */
@@ -235,12 +232,16 @@ public abstract class INode implements Diff.Element<byte[]> {
    *          otherwise, get the result from the current inode.
    * @return permission.
    */
-  public FsPermission getFsPermission(Snapshot snapshot) {
+  public final FsPermission getFsPermission(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).getFsPermission();
+    }
+
     return new FsPermission(
         (short)PermissionStatusFormat.MODE.retrieve(permission));
   }
   /** The same as getFsPermission(null). */
-  public FsPermission getFsPermission() {
+  public final FsPermission getFsPermission() {
     return getFsPermission(null);
   }
   protected short getFsPermissionShort() {
@@ -252,6 +253,14 @@ public abstract class INode implements Diff.Element<byte[]> {
     return updatePermissionStatus(PermissionStatusFormat.MODE, mode, latest);
   }
 
+  /**
+   * @return if the given snapshot is null, return this;
+   *     otherwise return the corresponding snapshot inode.
+   */
+  public INode getSnapshotINode(final Snapshot snapshot) {
+    return this;
+  }
+
   /** Is this inode in the latest snapshot? */
   public final boolean isInLatestSnapshot(final Snapshot latest) {
     return latest != null
@@ -427,7 +436,11 @@ public abstract class INode implements Diff.Element<byte[]> {
    *          otherwise, get the result from the current inode.
    * @return modification time.
    */
-  public long getModificationTime(Snapshot snapshot) {
+  public final long getModificationTime(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).modificationTime;
+    }
+
     return this.modificationTime;
   }
 
@@ -464,12 +477,16 @@ public abstract class INode implements Diff.Element<byte[]> {
    *          otherwise, get the result from the current inode.
    * @return access time
    */
-  public long getAccessTime(Snapshot snapshot) {
+  public final long getAccessTime(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).accessTime;
+    }
+
     return accessTime;
   }
 
   /** The same as getAccessTime(null). */
-  public long getAccessTime() {
+  public final long getAccessTime() {
     return getAccessTime(null);
   }
 
@@ -598,9 +615,7 @@ public abstract class INode implements Diff.Element<byte[]> {
     out.print(getObjectString());
     out.print("), parent=");
     out.print(parent == null? null: parent.getLocalName() + "/");
-    out.print(", permission=" + getFsPermission(snapshot));
+    out.print(", " + getPermissionStatus(snapshot));
-    out.print(", group=" + getGroupName(snapshot));
-    out.print(", user=" + getUserName(snapshot));
   }
   
   /**

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

@@ -834,21 +834,20 @@ public class INodeDirectory extends INode {
       prefix.setLength(prefix.length() - 2);
       prefix.append("  ");
     }
-    dumpTreeRecursively(out, prefix,
+    dumpTreeRecursively(out, prefix, new Iterable<SnapshotAndINode>() {
-        new Iterable<Pair<? extends INode, Snapshot>>() {
       final Iterator<INode> i = getChildrenList(snapshot).iterator();
       
       @Override
-      public Iterator<Pair<? extends INode, Snapshot>> iterator() {
+      public Iterator<SnapshotAndINode> iterator() {
-        return new Iterator<Pair<? extends INode, Snapshot>>() {
+        return new Iterator<SnapshotAndINode>() {
           @Override
           public boolean hasNext() {
             return i.hasNext();
           }
 
           @Override
-          public Pair<INode, Snapshot> next() {
+          public SnapshotAndINode next() {
-            return new Pair<INode, Snapshot>(i.next(), snapshot);
+            return new SnapshotAndINode(snapshot, i.next());
           }
 
           @Override
@@ -867,14 +866,29 @@ public class INodeDirectory extends INode {
    */
   @VisibleForTesting
   protected static void dumpTreeRecursively(PrintWriter out,
-      StringBuilder prefix, Iterable<Pair<? extends INode, Snapshot>> subs) {
+      StringBuilder prefix, Iterable<SnapshotAndINode> subs) {
     if (subs != null) {
-      for(final Iterator<Pair<? extends INode, Snapshot>> i = subs.iterator(); i.hasNext();) {
+      for(final Iterator<SnapshotAndINode> i = subs.iterator(); i.hasNext();) {
-        final Pair<? extends INode, Snapshot> pair = i.next();
+        final SnapshotAndINode pair = i.next();
         prefix.append(i.hasNext()? DUMPTREE_EXCEPT_LAST_ITEM: DUMPTREE_LAST_ITEM);
-        pair.left.dumpTreeRecursively(out, prefix, pair.right);
+        pair.inode.dumpTreeRecursively(out, prefix, pair.snapshot);
         prefix.setLength(prefix.length() - 2);
       }
     }
   }
+
+  /** A pair of Snapshot and INode objects. */
+  protected static class SnapshotAndINode {
+    public final Snapshot snapshot;
+    public final INode inode;
+
+    public SnapshotAndINode(Snapshot snapshot, INode inode) {
+      this.snapshot = snapshot;
+      this.inode = inode;
+    }
+
+    public SnapshotAndINode(Snapshot snapshot) {
+      this(snapshot, snapshot.getRoot());
+    }
+  }
 }

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

@@ -122,6 +122,11 @@ public class INodeFile extends INode implements BlockCollection {
         clientName, clientMachine, clientNode); 
   }
 
+  @Override
+  public INodeFile getSnapshotINode(final Snapshot snapshot) {
+    return this;
+  }
+
   @Override
   public INodeFile recordModification(final Snapshot latest) {
     return isInLatestSnapshot(latest)?
@@ -141,7 +146,11 @@ public class INodeFile extends INode implements BlockCollection {
   }
 
   /** @return the replication factor of the file. */
-  public short getFileReplication(Snapshot snapshot) {
+  public final short getFileReplication(Snapshot snapshot) {
+    if (snapshot != null) {
+      return getSnapshotINode(snapshot).getFileReplication();
+    }
+
     return HeaderFormat.getReplication(header);
   }
 

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

@@ -145,9 +145,15 @@ abstract class AbstractINodeDiffList<N extends INode,
     }
   }
 
-  N getSnapshotINode(Snapshot snapshot) {
+  /**
+   * @return the inode corresponding to the given snapshot.
+   *         Note that the current inode is returned if there is no change
+   *         between the given snapshot and the current state. 
+   */
+  N getSnapshotINode(final Snapshot snapshot, final N currentINode) {
     final D diff = getDiff(snapshot);
-    return diff == null? null: diff.getSnapshotINode();
+    final N inode = diff == null? null: diff.getSnapshotINode();
+    return inode == null? currentINode: inode;
   }
 
   /**

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

@@ -404,10 +404,10 @@ public class INodeDirectorySnapshottable extends INodeDirectoryWithSnapshot {
       final String name = getLocalName();
       out.println(name.isEmpty()? "/": name);
 
-      dumpTreeRecursively(out, prefix, new Iterable<Pair<? extends INode, Snapshot>>() {
+      dumpTreeRecursively(out, prefix, new Iterable<SnapshotAndINode>() {
         @Override
-        public Iterator<Pair<? extends INode, Snapshot>> iterator() {
+        public Iterator<SnapshotAndINode> iterator() {
-          return new Iterator<Pair<? extends INode, Snapshot>>() {
+          return new Iterator<SnapshotAndINode>() {
             final Iterator<DirectoryDiff> i = getDiffs().iterator();
             private DirectoryDiff next = findNext();
   
@@ -427,10 +427,9 @@ public class INodeDirectorySnapshottable extends INodeDirectoryWithSnapshot {
             }
   
             @Override
-            public Pair<INodeDirectory, Snapshot> next() {
+            public SnapshotAndINode next() {
               final Snapshot s = next.snapshot;
-              final Pair<INodeDirectory, Snapshot> pair =
+              final SnapshotAndINode pair = new SnapshotAndINode(s);
-                  new Pair<INodeDirectory, Snapshot>(s.getRoot(), s);
               next = findNext();
               return pair;
             }

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

@@ -26,7 +26,6 @@ import java.util.List;
 import java.util.Map;
 
 import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffReportEntry;
 import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffType;
 import org.apache.hadoop.hdfs.server.namenode.FSImageSerialization;
@@ -412,6 +411,11 @@ public class INodeDirectoryWithSnapshot extends INodeDirectoryWithQuota {
     return diffs;
   }
 
+  @Override
+  public INodeDirectory getSnapshotINode(Snapshot snapshot) {
+    return diffs.getSnapshotINode(snapshot, this);
+  }
+
   @Override
   public INodeDirectoryWithSnapshot recordModification(final Snapshot latest) {
     return isInLatestSnapshot(latest)?
@@ -507,37 +511,6 @@ public class INodeDirectoryWithSnapshot extends INodeDirectoryWithQuota {
     return diff != null? diff.getChild(name, true, this): super.getChild(name, null);
   }
 
-  @Override
-  public String getUserName(Snapshot snapshot) {
-    final INodeDirectory inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getUserName(): super.getUserName(null);
-  }
-
-  @Override
-  public String getGroupName(Snapshot snapshot) {
-    final INodeDirectory inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getGroupName(): super.getGroupName(null);
-  }
-
-  @Override
-  public FsPermission getFsPermission(Snapshot snapshot) {
-    final INodeDirectory inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getFsPermission(): super.getFsPermission(null);
-  }
-
-  @Override
-  public long getAccessTime(Snapshot snapshot) {
-    final INodeDirectory inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getAccessTime(): super.getAccessTime(null);
-  }
-
-  @Override
-  public long getModificationTime(Snapshot snapshot) {
-    final INodeDirectory inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getModificationTime()
-        : super.getModificationTime(null);
-  }
-
   @Override
   public String toDetailString() {
     return super.toDetailString() + ", " + diffs;

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

@@ -18,7 +18,6 @@
 package org.apache.hadoop.hdfs.server.namenode.snapshot;
 
 import org.apache.hadoop.classification.InterfaceAudience;
-import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.server.blockmanagement.DatanodeDescriptor;
 import org.apache.hadoop.hdfs.server.namenode.INodeFile;
 import org.apache.hadoop.hdfs.server.namenode.INodeFileUnderConstruction;
@@ -86,6 +85,11 @@ public class INodeFileUnderConstructionWithSnapshot
     return isCurrentFileDeleted;
   }
 
+  @Override
+  public INodeFile getSnapshotINode(Snapshot snapshot) {
+    return diffs.getSnapshotINode(snapshot, this);
+  }
+
   @Override
   public INodeFileUnderConstructionWithSnapshot recordModification(
       final Snapshot latest) {
@@ -105,13 +109,6 @@ public class INodeFileUnderConstructionWithSnapshot
     return diffs;
   }
 
-  @Override
-  public short getFileReplication(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getFileReplication()
-        : super.getFileReplication(null);
-  }
-
   @Override
   public short getBlockReplication() {
     return Util.getBlockReplication(this);
@@ -141,37 +138,6 @@ public class INodeFileUnderConstructionWithSnapshot
     return 1;
   }
 
-  @Override
-  public String getUserName(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getUserName(): super.getUserName(null);
-  }
-
-  @Override
-  public String getGroupName(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getGroupName(): super.getGroupName(null);
-  }
-
-  @Override
-  public FsPermission getFsPermission(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getFsPermission(): super.getFsPermission(null);
-  }
-
-  @Override
-  public long getAccessTime(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getAccessTime(): super.getAccessTime(null);
-  }
-
-  @Override
-  public long getModificationTime(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getModificationTime()
-        : super.getModificationTime(null);
-  }
-
   @Override
   public String toDetailString() {
     return super.toDetailString()

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

@@ -18,7 +18,6 @@
 package org.apache.hadoop.hdfs.server.namenode.snapshot;
 
 import org.apache.hadoop.classification.InterfaceAudience;
-import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.server.blockmanagement.DatanodeDescriptor;
 import org.apache.hadoop.hdfs.server.namenode.INodeFile;
 
@@ -57,6 +56,11 @@ public class INodeFileWithSnapshot extends INodeFile
     return isCurrentFileDeleted;
   }
 
+  @Override
+  public INodeFile getSnapshotINode(Snapshot snapshot) {
+    return diffs.getSnapshotINode(snapshot, this);
+  }
+
   @Override
   public INodeFileWithSnapshot recordModification(final Snapshot latest) {
     if (isInLatestSnapshot(latest)) {
@@ -75,13 +79,6 @@ public class INodeFileWithSnapshot extends INodeFile
     return diffs;
   }
 
-  @Override
-  public short getFileReplication(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getFileReplication()
-        : super.getFileReplication(null);
-  }
-
   @Override
   public short getBlockReplication() {
     return Util.getBlockReplication(this);
@@ -111,37 +108,6 @@ public class INodeFileWithSnapshot extends INodeFile
     return 1;
   }
 
-  @Override
-  public String getUserName(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getUserName(): super.getUserName(null);
-  }
-
-  @Override
-  public String getGroupName(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getGroupName(): super.getGroupName(null);
-  }
-
-  @Override
-  public FsPermission getFsPermission(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getFsPermission(): super.getFsPermission(null);
-  }
-
-  @Override
-  public long getAccessTime(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getAccessTime(): super.getAccessTime(null);
-  }
-
-  @Override
-  public long getModificationTime(Snapshot snapshot) {
-    final INodeFile inode = diffs.getSnapshotINode(snapshot);
-    return inode != null? inode.getModificationTime()
-        : super.getModificationTime(null);
-  }
-
   @Override
   public String toDetailString() {
     return super.toDetailString()