HDFS-4436. Change INode.recordModification(..) to return only the current inode and remove the updateCircularList parameter from some methods in INodeDirectoryWithSnapshot.Diff.

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

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

@@ -116,3 +116,7 @@ Branch-2802 Snapshot (Unreleased)
 
   HDFS-4126. Add reading/writing snapshot information to FSImage.
   (Jing Zhao via suresh)
+
+  HDFS-4436. Change INode.recordModification(..) to return only the current
+  inode and remove the updateCircularList parameter from some methods in
+  INodeDirectoryWithSnapshot.Diff.  (szetszwo)

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

@@ -1983,7 +1983,7 @@ LocatedBlock prepareFileForWrite(String src, INodeFile file,
       String leaseHolder, String clientMachine, DatanodeDescriptor clientNode,
       boolean writeToEditLog, Snapshot latestSnapshot) throws IOException {
     if (latestSnapshot != null) {
-      file = (INodeFile)file.recordModification(latestSnapshot).left;
+      file = file.recordModification(latestSnapshot);
     }
     final INodeFileUnderConstruction cons = file.toUnderConstruction(
         leaseHolder, clientMachine, clientNode);

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

@@ -242,8 +242,7 @@ public PermissionStatus getPermissionStatus() {
   }
   private INode updatePermissionStatus(PermissionStatusFormat f, long n,
       Snapshot latest) {
-    Pair<? extends INode, ? extends INode> pair = recordModification(latest);
+    final INode nodeToUpdate = recordModification(latest);
-    INode nodeToUpdate = pair != null ? pair.left : this;
     nodeToUpdate.permission = f.combine(n, permission);
     return nodeToUpdate;
   }
@@ -314,12 +313,14 @@ INode setPermission(FsPermission permission, Snapshot latest) {
    *
    * @param latest the latest snapshot that has been taken.
    *        Note that it is null if no snapshots have been taken.
-   * @return see {@link #createSnapshotCopy()}. 
+   * @return The current inode, which usually is the same object of this inode.
+   *         However, in some cases, this inode may be replaced with a new inode
+   *         for maintaining snapshots. The current inode is then the new inode.
    */
-  Pair<? extends INode, ? extends INode> recordModification(Snapshot latest) {
+  INode recordModification(final Snapshot latest) {
     Preconditions.checkState(!isDirectory(),
         "this is an INodeDirectory, this=%s", this);
-    return latest == null? null: parent.saveChild2Snapshot(this, latest);
+    return parent.saveChild2Snapshot(this, latest);
   }
 
   /**
@@ -489,8 +490,7 @@ void cloneModificationTime(INode that) {
    * Always set the last modification time of inode.
    */
   public INode setModificationTime(long modtime, Snapshot latest) {
-    Pair<? extends INode, ? extends INode> pair = recordModification(latest);
+    final INode nodeToUpdate = recordModification(latest);
-    INode nodeToUpdate = pair != null ? pair.left : this;
     nodeToUpdate.modificationTime = modtime;
     return nodeToUpdate;
   }
@@ -514,8 +514,7 @@ public long getAccessTime() {
    * Set last access time of inode.
    */
   public INode setAccessTime(long atime, Snapshot latest) {
-    Pair<? extends INode, ? extends INode> pair = recordModification(latest);
+    final INode nodeToUpdate = recordModification(latest);
-    INode nodeToUpdate = pair != null ? pair.left : this;    
     nodeToUpdate.accessTime = atime;
     return nodeToUpdate;
   }

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

@@ -171,7 +171,7 @@ INodeDirectoryWithQuota replaceSelf4Quota(final Snapshot latest,
           = INodeDirectoryWithSnapshot.newInstance(this, null);
       s.setQuota(nsQuota, dsQuota, null);
       replaceSelf(s);
-      s.save2Snapshot(latest, this);
+      s.saveSelf2Snapshot(latest, this);
       return s;
     }
   }
@@ -180,7 +180,7 @@ public INodeDirectorySnapshottable replaceSelf4INodeDirectorySnapshottable(
       Snapshot latest) {
     final INodeDirectorySnapshottable s = new INodeDirectorySnapshottable(this);
     replaceSelf(s);
-    s.save2Snapshot(latest, this);
+    s.saveSelf2Snapshot(latest, this);
     return s;
   }
 
@@ -229,25 +229,24 @@ INodeFileWithSnapshot replaceINodeFile(final INodeFile child) {
   }
 
   @Override
-  public Pair<? extends INode, ? extends INode> recordModification(Snapshot latest) {
+  public INodeDirectory recordModification(Snapshot latest) {
     if (latest == null) {
-      return null;
+      return this;
     }
-    return replaceSelf4INodeDirectoryWithSnapshot(latest)
+    final INodeDirectoryWithSnapshot withSnapshot
-        .save2Snapshot(latest, this);
+        = replaceSelf4INodeDirectoryWithSnapshot(latest);
+    withSnapshot.saveSelf2Snapshot(latest, this);
+    return withSnapshot;
   }
 
   /**
    * Save the child to the latest snapshot.
    * 
-   * @return a pair of inodes, where the left inode is the original child and
+   * @return the child inode, which may be replaced.
-   *         the right inode is the snapshot copy of the child; see also
-   *         {@link INode#createSnapshotCopy()}.
    */
-  public Pair<? extends INode, ? extends INode> saveChild2Snapshot(
+  public INode saveChild2Snapshot(INode child, Snapshot latest) {
-      INode child, Snapshot latest) {
     if (latest == null) {
-      return null;
+      return child;
     }
     return replaceSelf4INodeDirectoryWithSnapshot(latest)
         .saveChild2Snapshot(child, latest);

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

@@ -111,6 +111,12 @@ protected INodeFile(INodeFile that) {
     this.blocks = that.blocks;
   }
 
+  @Override
+  INodeFile recordModification(final Snapshot latest) {
+    //TODO: change it to use diff list
+    return (INodeFile)super.recordModification(latest);
+  }
+
   @Override
   public Pair<? extends INodeFile, ? extends INodeFile> createSnapshotCopy() {
     return parent.replaceINodeFile(this).createSnapshotCopy();
@@ -155,13 +161,9 @@ public short getBlockReplication() {
 
   public void setFileReplication(short replication, Snapshot latest) {
     if (latest != null) {
-      final Pair<? extends INode, ? extends INode> p = recordModification(latest);
+      recordModification(latest).setFileReplication(replication, null);
-      if (p != null) {
-        ((INodeFile)p.left).setFileReplication(replication, null);
       return;
     }
-    }
-
     header = HeaderFormat.combineReplication(header, replication);
   }
 

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

@@ -64,13 +64,16 @@ static FileWithSnapshot getPrevious(FileWithSnapshot file) {
     
     /** Replace the old file with the new file in the circular linked list. */
     static void replace(FileWithSnapshot oldFile, FileWithSnapshot newFile) {
-      //set next element
+      final FileWithSnapshot oldNext = oldFile.getNext();
-      FileWithSnapshot i = oldFile.getNext();
+      if (oldNext == null) {
-      newFile.setNext(i);
+        newFile.setNext(null);
+      } else {
+        if (oldNext != oldFile) {
+          newFile.setNext(oldNext);
+          getPrevious(oldFile).setNext(newFile);
+        }
         oldFile.setNext(null);
-      //find previous element and update it
+      }
-      for(; i.getNext() != oldFile; i = i.getNext());
-      i.setNext(newFile);
     }
 
     /**

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

@@ -169,18 +169,13 @@ void undoCreate(final INode inode, final int insertionPoint) {
      * Delete an inode from current state.
      * @return a triple for undo.
      */
-    Triple<Integer, INode, Integer> delete(final INode inode,
+    Triple<Integer, INode, Integer> delete(final INode inode) {
-        boolean updateCircularList) {
       final int c = search(created, inode);
       INode previous = null;
       Integer d = null;
       if (c >= 0) {
         // remove a newly created inode
         previous = created.remove(c);
-        if (updateCircularList && previous instanceof FileWithSnapshot) {
-          // also we should remove previous from the circular list
-          ((FileWithSnapshot) previous).removeSelf();
-        }
       } else {
         // not in c-list, it must be in previous
         d = search(deleted, inode);
@@ -204,7 +199,7 @@ void undoDelete(final INode inode,
      * @return a triple for undo.
      */
     Triple<Integer, INode, Integer> modify(final INode oldinode,
-        final INode newinode, boolean updateCircularList) {
+        final INode newinode) {
       if (!oldinode.equals(newinode)) {
         throw new AssertionError("The names do not match: oldinode="
             + oldinode + ", newinode=" + newinode);
@@ -216,14 +211,6 @@ Triple<Integer, INode, Integer> modify(final INode oldinode,
         // Case 1.1.3 and 2.3.3: inode is already in c-list,
         previous = created.set(c, newinode);
         
-        if (updateCircularList && newinode instanceof FileWithSnapshot) {
-          // also should remove oldinode from the circular list
-          FileWithSnapshot newNodeWithLink = (FileWithSnapshot) newinode;
-          FileWithSnapshot oldNodeWithLink = (FileWithSnapshot) oldinode;
-          newNodeWithLink.setNext(oldNodeWithLink.getNext());
-          oldNodeWithLink.setNext(null);
-        }
-        
         //TODO: fix a bug that previous != oldinode.  Set it to oldinode for now
         previous = oldinode;
       } else {
@@ -356,11 +343,8 @@ List<INode> apply2Current(final List<INode> current) {
      * @param the posterior diff to combine
      * @param deletedINodeProcesser Used in case 2.1, 2.3, 3.1, and 3.3
      *                              to process the deleted inodes.
-     * @param updateCircularList Whether to update the circular linked list 
-     *                           while combining the diffs.                             
      */
-    void combinePostDiff(Diff postDiff, Processor deletedINodeProcesser,
+    void combinePostDiff(Diff postDiff, Processor deletedINodeProcesser) {
-        boolean updateCircularList) {
       final List<INode> postCreated = postDiff.created != null?
           postDiff.created: Collections.<INode>emptyList();
       final List<INode> postDeleted = postDiff.deleted != null?
@@ -381,16 +365,14 @@ void combinePostDiff(Diff postDiff, Processor deletedINodeProcesser,
           c = createdIterator.hasNext()? createdIterator.next(): null;
         } else if (cmp > 0) {
           // case 2: only in d-list
-          Triple<Integer, INode, Integer> triple = delete(d, 
+          Triple<Integer, INode, Integer> triple = delete(d);
-              updateCircularList);
           if (deletedINodeProcesser != null) {
             deletedINodeProcesser.process(triple.middle);
           }
           d = deletedIterator.hasNext()? deletedIterator.next(): null;
         } else {
           // case 3: in both c-list and d-list 
-          final Triple<Integer, INode, Integer> triple = modify(d, c,
+          final Triple<Integer, INode, Integer> triple = modify(d, c);
-              updateCircularList);
           if (deletedINodeProcesser != null) {
             deletedINodeProcesser.process(triple.middle);
           }
@@ -562,18 +544,14 @@ boolean isSnapshotRoot() {
     }
 
     /** Copy the INode state to the snapshot if it is not done already. */
-    private Pair<INodeDirectory, INodeDirectory> checkAndInitINode(
+    private void checkAndInitINode(INodeDirectory snapshotCopy) {
-        INodeDirectory snapshotCopy) {
+      if (snapshotINode == null) {
-      if (snapshotINode != null) {
-        // already initialized.
-        return null;
-      }
-      final INodeDirectoryWithSnapshot dir = INodeDirectoryWithSnapshot.this;
         if (snapshotCopy == null) {
-        snapshotCopy = new INodeDirectory(dir, false);
+          snapshotCopy = new INodeDirectory(INodeDirectoryWithSnapshot.this,
+              false);
         }
         snapshotINode = snapshotCopy;
-      return new Pair<INodeDirectory, INodeDirectory>(dir, snapshotCopy);
+      }
     }
 
     /** @return the snapshot object of this diff. */
@@ -605,7 +583,7 @@ private List<INode> initChildren() {
           if (children == null) {
             final Diff combined = new Diff();
             for(SnapshotDiff d = SnapshotDiff.this; d != null; d = d.posteriorDiff) {
-              combined.combinePostDiff(d.diff, null, false);
+              combined.combinePostDiff(d.diff, null);
             }
             children = combined.apply2Current(ReadOnlyList.Util.asList(
                 INodeDirectoryWithSnapshot.this.getChildrenList(null)));
@@ -748,7 +726,7 @@ public void process(INode inode) {
               ((INodeFile)inode).collectSubtreeBlocksAndClear(collectedBlocks);
             }
           }
-        }, true);
+        });
 
         previousDiff.posteriorDiff = diffToRemove.posteriorDiff;
         diffToRemove.posteriorDiff = null;
@@ -839,32 +817,44 @@ SnapshotDiff getSnapshotDiff(Snapshot snapshot) {
   }
 
   @Override
-  public Pair<INodeDirectory, INodeDirectory> recordModification(Snapshot latest) {
+  public INodeDirectoryWithSnapshot recordModification(Snapshot latest) {
-    return save2Snapshot(latest, null);
+    saveSelf2Snapshot(latest, null);
+    return this;
   }
 
   /** Save the snapshot copy to the latest snapshot. */
-  public Pair<INodeDirectory, INodeDirectory> save2Snapshot(Snapshot latest,
+  public void saveSelf2Snapshot(Snapshot latest, INodeDirectory snapshotCopy) {
-      INodeDirectory snapshotCopy) {
+    if (latest != null) {
-    return latest == null? null
+      checkAndAddLatestSnapshotDiff(latest).checkAndInitINode(snapshotCopy);
-        : checkAndAddLatestSnapshotDiff(latest).checkAndInitINode(snapshotCopy);
+    }
   }
 
   @Override
-  public Pair<? extends INode, ? extends INode> saveChild2Snapshot(
+  public INode saveChild2Snapshot(INode child, Snapshot latest) {
-      INode child, Snapshot latest) {
     Preconditions.checkArgument(!child.isDirectory(),
         "child is a directory, child=%s", child);
+    if (latest == null) {
+      return child;
+    }
 
     final SnapshotDiff diff = checkAndAddLatestSnapshotDiff(latest);
     if (diff.getChild(child.getLocalNameBytes(), false) != null) {
       // it was already saved in the latest snapshot earlier.  
-      return null;
+      return child;
     }
 
     final Pair<? extends INode, ? extends INode> p = child.createSnapshotCopy();
-    diff.diff.modify(p.right, p.left, true);
+    if (p.left != p.right) {
-    return p;
+      final Triple<Integer, INode, Integer> triple = diff.diff.modify(p.right, p.left);
+      if (triple.middle != null && p.left instanceof FileWithSnapshot) {
+        // also should remove oldinode from the circular list
+        FileWithSnapshot newNodeWithLink = (FileWithSnapshot) p.left;
+        FileWithSnapshot oldNodeWithLink = (FileWithSnapshot) p.right;
+        newNodeWithLink.setNext(oldNodeWithLink.getNext());
+        oldNodeWithLink.setNext(null);
+      }
+    }
+    return p.left;
   }
 
   @Override
@@ -888,11 +878,20 @@ public INode removeChild(INode child, Snapshot latest) {
     Triple<Integer, INode, Integer> undoInfo = null;
     if (latest != null) {
       diff = checkAndAddLatestDiff(latest);
-      undoInfo = diff.delete(child, true);
+      undoInfo = diff.delete(child);
     }
     final INode removed = super.removeChild(child, null);
-    if (removed == null && undoInfo != null) {
+    if (undoInfo != null) {
+      if (removed == null) {
+        //remove failed, undo
         diff.undoDelete(child, undoInfo);
+      } else {
+        //clean up the previously created file, if there is any.
+        final INode previous = undoInfo.middle;
+        if (previous != null && previous instanceof FileWithSnapshot) {
+          ((FileWithSnapshot)previous).removeSelf();
+        }
+      }
     }
     return removed;
   }

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

@@ -148,7 +148,7 @@ void runDiffTest(int startSize, int numModifications) {
     // combine all diffs
     final Diff combined = diffs[0];
     for(int i = 1; i < diffs.length; i++) {
-      combined.combinePostDiff(diffs[i], null, false);
+      combined.combinePostDiff(diffs[i], null);
     }
 
     {
@@ -284,7 +284,7 @@ static void delete(INode inode, final List<INode> current, Diff diff) {
         before = toString(diff);
       }
 
-      final Triple<Integer, INode, Integer> undoInfo = diff.delete(inode, true);
+      final Triple<Integer, INode, Integer> undoInfo = diff.delete(inode);
 
       if (testUndo) {
         final String after = toString(diff);
@@ -292,7 +292,7 @@ static void delete(INode inode, final List<INode> current, Diff diff) {
         diff.undoDelete(inode, undoInfo);
         assertDiff(before, diff);
         //re-do
-        diff.delete(inode, true);
+        diff.delete(inode);
         assertDiff(after, diff);
       }
     }
@@ -314,7 +314,7 @@ static void modify(INode inode, final List<INode> current, Diff diff) {
         before = toString(diff);
       }
 
-      final Triple<Integer, INode, Integer> undoInfo = diff.modify(oldinode, newinode, true);
+      final Triple<Integer, INode, Integer> undoInfo = diff.modify(oldinode, newinode);
 
       if (testUndo) {
         final String after = toString(diff);
@@ -322,7 +322,7 @@ static void modify(INode inode, final List<INode> current, Diff diff) {
         diff.undoModify(oldinode, newinode, undoInfo);
         assertDiff(before, diff);
         //re-do
-        diff.modify(oldinode, newinode, true);
+        diff.modify(oldinode, newinode);
         assertDiff(after, diff);
       }
     }