HDFS-4206. Change the fields in INode and its subclasses to private.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1410996 13f79535-47bb-0310-9956-ffa450edef68

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

@@ -161,6 +161,9 @@ Trunk (Unreleased)
 
     HDFS-3935. Add JournalNode to the start/stop scripts (Andy Isaacson via todd)
 
+    HDFS-4206. Change the fields in INode and its subclasses to private.
+    (szetszwo)
+
   OPTIMIZATIONS
 
   BUG FIXES

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

@@ -306,14 +306,14 @@ INode unprotectedAddFile( String path,
     return newNode;
   }
 
-  INodeDirectory addToParent(byte[] src, INodeDirectory parentINode,
+  INodeDirectory addToParent(INodeDirectory parentINode,
       INode newNode, boolean propagateModTime) {
     // NOTE: This does not update space counts for parents
     INodeDirectory newParent = null;
     writeLock();
     try {
       try {
-        newParent = rootDir.addToParent(src, newNode, parentINode,
+        newParent = rootDir.addToParent(newNode, parentINode,
                                         propagateModTime);
         cacheName(newNode);
       } catch (FileNotFoundException e) {
@@ -540,7 +540,7 @@ boolean unprotectedRenameTo(String src, String dst, long timestamp)
       return true;
     }
     if (srcInode.isSymlink() && 
-        dst.equals(((INodeSymlink)srcInode).getLinkValue())) {
+        dst.equals(((INodeSymlink)srcInode).getSymlinkString())) {
       throw new FileAlreadyExistsException(
           "Cannot rename symlink "+src+" to its target "+dst);
     }
@@ -663,7 +663,7 @@ boolean unprotectedRenameTo(String src, String dst, long timestamp,
           "The source "+src+" and destination "+dst+" are the same");
     }
     if (srcInode.isSymlink() && 
-        dst.equals(((INodeSymlink)srcInode).getLinkValue())) {
+        dst.equals(((INodeSymlink)srcInode).getSymlinkString())) {
       throw new FileAlreadyExistsException(
           "Cannot rename symlink "+src+" to its target "+dst);
     }
@@ -702,16 +702,17 @@ boolean unprotectedRenameTo(String src, String dst, long timestamp,
             + error);
         throw new FileAlreadyExistsException(error);
       }
-      List<INode> children = dstInode.isDirectory() ? 
+      if (dstInode.isDirectory()) {
-          ((INodeDirectory) dstInode).getChildren() : null;
+        final List<INode> children = ((INodeDirectory) dstInode
-      if (children != null && children.size() != 0) {
+            ).getChildrenList();
-        error = "rename cannot overwrite non empty destination directory "
+        if (!children.isEmpty()) {
-            + dst;
+          error = "rename destination directory is not empty: " + dst;
-        NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
+          NameNode.stateChangeLog.warn(
-            + error);
+              "DIR* FSDirectory.unprotectedRenameTo: " + error);
           throw new IOException(error);
         }
       }
+    }
     if (dstInodes[dstInodes.length - 2] == null) {
       error = "rename destination parent " + dst + " not found.";
       NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
@@ -1174,7 +1175,7 @@ DirectoryListing getListing(String src, byte[] startAfter,
       HdfsFileStatus listing[] = new HdfsFileStatus[numOfListing];
       for (int i=0; i<numOfListing; i++) {
         INode cur = contents.get(startChild+i);
-        listing[i] = createFileStatus(cur.name, cur, needLocation);
+        listing[i] = createFileStatus(cur.getLocalNameBytes(), cur, needLocation);
       }
       return new DirectoryListing(
           listing, totalNumChildren-startChild-numOfListing);
@@ -1371,7 +1372,7 @@ private void unprotectedUpdateCount(INodesInPath inodesInPath,
     for(int i=0; i < numOfINodes; i++) {
       if (inodes[i].isQuotaSet()) { // a directory with quota
         INodeDirectoryWithQuota node =(INodeDirectoryWithQuota)inodes[i]; 
-        node.unprotectedUpdateNumItemsInTree(nsDelta, dsDelta);
+        node.addSpaceConsumed(nsDelta, dsDelta);
       }
     }
   }

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

@@ -257,7 +257,8 @@ private int loadDirectory(DataInputStream in) throws IOException {
        INode newNode = loadINode(in); // read rest of inode
 
        // add to parent
-       namesystem.dir.addToParent(localName, parent, newNode, false);
+       newNode.setLocalName(localName);
+       namesystem.dir.addToParent(parent, newNode, false);
      }
      return numChildren;
    }
@@ -291,8 +292,8 @@ private void loadFullNameINodes(long numFiles,
       }
 
       // add new inode
-      parentINode = fsDir.addToParent(pathComponents[pathComponents.length-1], 
+      newNode.setLocalName(pathComponents[pathComponents.length-1]);
-          parentINode, newNode, false);
+      parentINode = fsDir.addToParent(parentINode, newNode, false);
     }
   }
 
@@ -530,8 +531,8 @@ void save(File newFile,
     private void saveImage(ByteBuffer currentDirName,
                                   INodeDirectory current,
                                   DataOutputStream out) throws IOException {
-      List<INode> children = current.getChildren();
+      final List<INode> children = current.getChildrenList();
-      if (children == null || children.isEmpty())
+      if (children.isEmpty())
         return;
       // print prefix (parent directory name)
       int prefixLen = currentDirName.position();

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

@@ -168,7 +168,7 @@ static void saveINode2Image(INode node,
       out.writeLong(0);   // access time
       out.writeLong(0);   // preferred block size
       out.writeInt(-2);   // # of blocks
-      Text.writeString(out, ((INodeSymlink)node).getLinkValue());
+      Text.writeString(out, ((INodeSymlink)node).getSymlinkString());
       filePerm.fromShort(node.getFsPermissionShort());
       PermissionStatus.write(out, node.getUserName(),
                              node.getGroupName(),

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

@@ -45,23 +45,12 @@
 @InterfaceAudience.Private
 abstract class INode implements Comparable<byte[]> {
   static final List<INode> EMPTY_LIST = Collections.unmodifiableList(new ArrayList<INode>());
-  /**
-   *  The inode name is in java UTF8 encoding; 
-   *  The name in HdfsFileStatus should keep the same encoding as this.
-   *  if this encoding is changed, implicitly getFileInfo and listStatus in
-   *  clientProtocol are changed; The decoding at the client
-   *  side should change accordingly.
-   */
-  protected byte[] name;
-  protected INodeDirectory parent;
-  protected long modificationTime;
-  protected long accessTime;
 
-  /** Simple wrapper for two counters : 
+  /** Wrapper of two counters for namespace consumed and diskspace consumed. */
-   *  nsCount (namespace consumed) and dsCount (diskspace consumed).
-   */
   static class DirCounts {
+    /** namespace count */
     long nsCount = 0;
+    /** diskspace count */
     long dsCount = 0;
     
     /** returns namespace count */
@@ -74,10 +63,6 @@ long getDsCount() {
     }
   }
   
-  //Only updated by updatePermissionStatus(...).
-  //Other codes should not modify it.
-  private long permission;
-
   private static enum PermissionStatusFormat {
     MODE(0, 16),
     GROUP(MODE.OFFSET + MODE.LENGTH, 25),
@@ -100,31 +85,67 @@ long retrieve(long record) {
     long combine(long bits, long record) {
       return (record & ~MASK) | (bits << OFFSET);
     }
+
+    /** Set the {@link PermissionStatus} */
+    static long toLong(PermissionStatus ps) {
+      long permission = 0L;
+      final int user = SerialNumberManager.INSTANCE.getUserSerialNumber(
+          ps.getUserName());
+      permission = PermissionStatusFormat.USER.combine(user, permission);
+      final int group = SerialNumberManager.INSTANCE.getGroupSerialNumber(
+          ps.getGroupName());
+      permission = PermissionStatusFormat.GROUP.combine(group, permission);
+      final int mode = ps.getPermission().toShort();
+      permission = PermissionStatusFormat.MODE.combine(mode, permission);
+      return permission;
+    }
   }
 
-  INode(PermissionStatus permissions, long mTime, long atime) {
+  /**
-    this.name = null;
+   *  The inode name is in java UTF8 encoding; 
-    this.parent = null;
+   *  The name in HdfsFileStatus should keep the same encoding as this.
-    this.modificationTime = mTime;
+   *  if this encoding is changed, implicitly getFileInfo and listStatus in
-    setAccessTime(atime);
+   *  clientProtocol are changed; The decoding at the client
-    setPermissionStatus(permissions);
+   *  side should change accordingly.
+   */
+  private byte[] name = null;
+  /** 
+   * Permission encoded using PermissionStatusFormat.
+   * Codes other than {@link #updatePermissionStatus(PermissionStatusFormat, long)}.
+   * should not modify it.
+   */
+  private long permission = 0L;
+  protected INodeDirectory parent = null;
+  protected long modificationTime = 0L;
+  protected long accessTime = 0L;
+
+  private INode(byte[] name, long permission, INodeDirectory parent,
+      long modificationTime, long accessTime) {
+    this.name = name;
+    this.permission = permission;
+    this.parent = parent;
+    this.modificationTime = modificationTime;
+    this.accessTime = accessTime;
+  }
+
+  INode(byte[] name, PermissionStatus permissions, INodeDirectory parent,
+      long modificationTime, long accessTime) {
+    this(name, PermissionStatusFormat.toLong(permissions), parent,
+        modificationTime, accessTime);
+  }
+
+  INode(PermissionStatus permissions, long mtime, long atime) {
+    this(null, permissions, null, mtime, atime);
   }
 
   protected INode(String name, PermissionStatus permissions) {
-    this(permissions, 0L, 0L);
+    this(DFSUtil.string2Bytes(name), permissions, null, 0L, 0L);
-    setLocalName(name);
   }
   
-  /** copy constructor
+  /** @param other Other node to be copied */
-   * 
-   * @param other Other node to be copied
-   */
   INode(INode other) {
-    setLocalName(other.getLocalName());
+    this(other.getLocalNameBytes(), other.permission, other.getParent(), 
-    this.parent = other.getParent();
+        other.getModificationTime(), other.getAccessTime());
-    setPermissionStatus(other.getPermissionStatus());
-    setModificationTime(other.getModificationTime());
-    setAccessTime(other.getAccessTime());
   }
 
   /**

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

@@ -50,22 +50,19 @@ public static INodeDirectory valueOf(INode inode, String path
   protected static final int DEFAULT_FILES_PER_DIRECTORY = 5;
   final static String ROOT_NAME = "";
 
-  private List<INode> children;
+  private List<INode> children = null;
 
   INodeDirectory(String name, PermissionStatus permissions) {
     super(name, permissions);
-    this.children = null;
   }
 
   public INodeDirectory(PermissionStatus permissions, long mTime) {
     super(permissions, mTime, 0);
-    this.children = null;
   }
 
   /** constructor */
-  INodeDirectory(byte[] localName, PermissionStatus permissions, long mTime) {
+  INodeDirectory(byte[] name, PermissionStatus permissions, long mtime) {
-    this(permissions, mTime);
+    super(name, permissions, null, mtime, 0L);
-    this.name = localName;
   }
   
   /** copy constructor
@@ -74,7 +71,7 @@ public INodeDirectory(PermissionStatus permissions, long mTime) {
    */
   INodeDirectory(INodeDirectory other) {
     super(other);
-    this.children = other.getChildren();
+    this.children = other.children;
   }
   
   /** @return true unconditionally. */
@@ -83,25 +80,30 @@ public final boolean isDirectory() {
     return true;
   }
 
-  INode removeChild(INode node) {
+  private void assertChildrenNonNull() {
-    assert children != null;
+    if (children == null) {
-    int low = Collections.binarySearch(children, node.name);
+      throw new AssertionError("children is null: " + this);
-    if (low >= 0) {
-      return children.remove(low);
-    } else {
-      return null;
     }
   }
 
+  private int searchChildren(INode inode) {
+    return Collections.binarySearch(children, inode.getLocalNameBytes());
+  }
+
+  INode removeChild(INode node) {
+    assertChildrenNonNull();
+    final int i = searchChildren(node);
+    return i >= 0? children.remove(i): null;
+  }
+
   /** Replace a child that has the same name as newChild by newChild.
    * 
    * @param newChild Child node to be added
    */
   void replaceChild(INode newChild) {
-    if ( children == null ) {
+    assertChildrenNonNull();
-      throw new IllegalArgumentException("The directory is empty");
+
-    }
+    final int low = searchChildren(newChild);
-    int low = Collections.binarySearch(children, newChild.name);
     if (low>=0) { // an old child exists so replace by the newChild
       children.set(low, newChild);
     } else {
@@ -210,7 +212,7 @@ INodesInPath getExistingPathINodes(byte[][] components, int numOfINodes,
         final String remainder =
           constructPath(components, count + 1, components.length);
         final String link = DFSUtil.bytes2String(components[count]);
-        final String target = ((INodeSymlink)curNode).getLinkValue();
+        final String target = ((INodeSymlink)curNode).getSymlinkString();
         if (NameNode.stateChangeLog.isDebugEnabled()) {
           NameNode.stateChangeLog.debug("UnresolvedPathException " +
             " path: " + path + " preceding: " + preceding +
@@ -282,7 +284,7 @@ <T extends INode> T addChild(final T node, boolean setModTime) {
     if (children == null) {
       children = new ArrayList<INode>(DEFAULT_FILES_PER_DIRECTORY);
     }
-    int low = Collections.binarySearch(children, node.name);
+    final int low = searchChildren(node);
     if(low >= 0)
       return null;
     node.parent = this;
@@ -322,13 +324,9 @@ <T extends INode> T addNode(String path, T newNode
    * @throws  FileNotFoundException if parent does not exist or 
    *          is not a directory.
    */
-  INodeDirectory addToParent( byte[] localname,
+  INodeDirectory addToParent(INode newNode, INodeDirectory parent,
-                              INode newNode,
+      boolean propagateModTime) throws FileNotFoundException {
-                              INodeDirectory parent,
-                              boolean propagateModTime
-                              ) throws FileNotFoundException {
     // insert into the parent children list
-    newNode.name = localname;
     if(parent.addChild(newNode, propagateModTime) == null)
       return null;
     return parent;
@@ -366,7 +364,7 @@ INodeDirectory addToParent(byte[][] pathComponents, INode newNode,
     if (pathComponents.length < 2) { // add root
       return null;
     }
-    newNode.name = pathComponents[pathComponents.length - 1];
+    newNode.setLocalName(pathComponents[pathComponents.length - 1]);
     // insert into the parent children list
     INodeDirectory parent = getParent(pathComponents);
     return parent.addChild(newNode, propagateModTime) == null? null: parent;
@@ -422,10 +420,6 @@ long[] computeContentSummary(long[] summary) {
   public List<INode> getChildrenList() {
     return children==null ? EMPTY_LIST : children;
   }
-  /** @return the children list which is possibly null. */
-  public List<INode> getChildren() {
-    return children;
-  }
 
   @Override
   int collectSubtreeBlocksAndClear(BlocksMapUpdateInfo info) {

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

@@ -27,9 +27,9 @@
  */
 class INodeDirectoryWithQuota extends INodeDirectory {
   private long nsQuota; /// NameSpace quota
-  private long nsCount;
+  private long nsCount = 1L;
   private long dsQuota; /// disk space quota
-  private long diskspace;
+  private long diskspace = 0L;
   
   /** Convert an existing directory inode to one with the given quota
    * 
@@ -44,7 +44,8 @@ class INodeDirectoryWithQuota extends INodeDirectory {
     other.spaceConsumedInTree(counts);
     this.nsCount = counts.getNsCount();
     this.diskspace = counts.getDsCount();
-    setQuota(nsQuota, dsQuota);
+    this.nsQuota = nsQuota;
+    this.dsQuota = dsQuota;
   }
   
   /** constructor with no quota verification */
@@ -53,7 +54,6 @@ class INodeDirectoryWithQuota extends INodeDirectory {
     super(permissions, modificationTime);
     this.nsQuota = nsQuota;
     this.dsQuota = dsQuota;
-    this.nsCount = 1;
   }
   
   /** constructor with no quota verification */
@@ -62,7 +62,6 @@ class INodeDirectoryWithQuota extends INodeDirectory {
     super(name, permissions);
     this.nsQuota = nsQuota;
     this.dsQuota = dsQuota;
-    this.nsCount = 1;
   }
   
   /** Get this directory's namespace quota
@@ -116,19 +115,8 @@ long diskspaceConsumed() {
    * @param nsDelta the change of the tree size
    * @param dsDelta change to disk space occupied
    */
-  void updateNumItemsInTree(long nsDelta, long dsDelta) {
+  void addSpaceConsumed(long nsDelta, long dsDelta) {
-    nsCount += nsDelta;
+    setSpaceConsumed(nsCount + nsDelta, diskspace + dsDelta);
-    diskspace += dsDelta;
-  }
-  
-  /** Update the size of the tree
-   * 
-   * @param nsDelta the change of the tree size
-   * @param dsDelta change to disk space occupied
-   **/
-  void unprotectedUpdateNumItemsInTree(long nsDelta, long dsDelta) {
-    nsCount = nsCount + nsDelta;
-    diskspace = diskspace + dsDelta;
   }
   
   /** 

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

@@ -45,14 +45,43 @@ public static INodeFile valueOf(INode inode, String path) throws IOException {
 
   static final FsPermission UMASK = FsPermission.createImmutable((short)0111);
 
-  //Number of bits for Block size
-  static final short BLOCKBITS = 48;
 
-  //Header mask 64-bit representation
+  /** Format: [16 bits for replication][48 bits for PreferredBlockSize] */
-  //Format: [16 bits for replication][48 bits for PreferredBlockSize]
+  private static class HeaderFormat {
+    /** Number of bits for Block size */
+    static final int BLOCKBITS = 48;
+    /** Header mask 64-bit representation */
     static final long HEADERMASK = 0xffffL << BLOCKBITS;
+    static final long MAX_BLOCK_SIZE = ~HEADERMASK; 
     
-  private long header;
+    static short getReplication(long header) {
+      return (short) ((header & HEADERMASK) >> BLOCKBITS);
+    }
+
+    static long combineReplication(long header, short replication) {
+      if (replication <= 0) {
+         throw new IllegalArgumentException(
+             "Unexpected value for the replication: " + replication);
+      }
+      return ((long)replication << BLOCKBITS) | (header & MAX_BLOCK_SIZE);
+    }
+    
+    static long getPreferredBlockSize(long header) {
+      return header & MAX_BLOCK_SIZE;
+    }
+
+    static long combinePreferredBlockSize(long header, long blockSize) {
+      if (blockSize < 0) {
+         throw new IllegalArgumentException("Block size < 0: " + blockSize);
+      } else if (blockSize > MAX_BLOCK_SIZE) {
+        throw new IllegalArgumentException("Block size = " + blockSize
+            + " > MAX_BLOCK_SIZE = " + MAX_BLOCK_SIZE);
+     }
+      return (header & HEADERMASK) | (blockSize & MAX_BLOCK_SIZE);
+    }
+  }
+
+  private long header = 0L;
 
   private BlockInfo[] blocks;
 
@@ -60,8 +89,8 @@ public static INodeFile valueOf(INode inode, String path) throws IOException {
                       short replication, long modificationTime,
                       long atime, long preferredBlockSize) {
     super(permissions, modificationTime, atime);
-    this.setReplication(replication);
+    header = HeaderFormat.combineReplication(header, replication);
-    this.setPreferredBlockSize(preferredBlockSize);
+    header = HeaderFormat.combinePreferredBlockSize(header, preferredBlockSize);
     this.blocks = blklist;
   }
 
@@ -78,25 +107,17 @@ void setPermission(FsPermission permission) {
   /** @return the replication factor of the file. */
   @Override
   public short getBlockReplication() {
-    return (short) ((header & HEADERMASK) >> BLOCKBITS);
+    return HeaderFormat.getReplication(header);
   }
 
   void setReplication(short replication) {
-    if(replication <= 0)
+    header = HeaderFormat.combineReplication(header, replication);
-       throw new IllegalArgumentException("Unexpected value for the replication");
-    header = ((long)replication << BLOCKBITS) | (header & ~HEADERMASK);
   }
 
   /** @return preferred block size (in bytes) of the file. */
   @Override
   public long getPreferredBlockSize() {
-    return header & ~HEADERMASK;
+    return HeaderFormat.getPreferredBlockSize(header);
-  }
-
-  private void setPreferredBlockSize(long preferredBlkSize) {
-    if((preferredBlkSize < 0) || (preferredBlkSize > ~HEADERMASK ))
-       throw new IllegalArgumentException("Unexpected value for the block size");
-    header = (header & HEADERMASK) | (preferredBlkSize & ~HEADERMASK);
   }
 
   /** @return the blocks of the file. */

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

@@ -22,19 +22,16 @@
 import org.apache.hadoop.hdfs.DFSUtil;
 
 /**
- * An INode representing a symbolic link.
+ * An {@link INode} representing a symbolic link.
  */
 @InterfaceAudience.Private
 public class INodeSymlink extends INode {
-  private byte[] symlink; // The target URI
+  private final byte[] symlink; // The target URI
 
-  INodeSymlink(String value, long modTime, long atime,
+  INodeSymlink(String value, long mtime, long atime,
                PermissionStatus permissions) {
-    super(permissions, modTime, atime);
+    super(permissions, mtime, atime);
-    assert value != null;
+    this.symlink = DFSUtil.string2Bytes(value);
-    setLinkValue(value);
-    setModificationTimeForce(modTime);
-    setAccessTime(atime);
   }
 
   @Override
@@ -42,11 +39,7 @@ public boolean isSymlink() {
     return true;
   }
 
-  void setLinkValue(String value) {
+  public String getSymlinkString() {
-    this.symlink = DFSUtil.string2Bytes(value);
-  }
-
-  public String getLinkValue() {
     return DFSUtil.bytes2String(symlink);
   }
 

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java

@@ -134,7 +134,7 @@ public void testReset() throws Exception {
     fsdir.reset();
     Assert.assertFalse(fsdir.isReady());
     final INodeDirectory root = (INodeDirectory) fsdir.getINode("/");
-    Assert.assertNull(root.getChildren());
+    Assert.assertTrue(root.getChildrenList().isEmpty());
     fsdir.imageLoadComplete();
     Assert.assertTrue(fsdir.isReady());
   }