From bb80f2fb29d6f58d9c35f4a1fd88c99517f43e16 Mon Sep 17 00:00:00 2001 From: Tsz-wo Sze Date: Thu, 24 Jan 2013 21:33:34 +0000 Subject: [PATCH] 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/CHANGES.HDFS-2802.txt | 4 + .../hdfs/server/namenode/FSNamesystem.java | 2 +- .../hadoop/hdfs/server/namenode/INode.java | 17 ++-- .../hdfs/server/namenode/INodeDirectory.java | 23 +++-- .../hdfs/server/namenode/INodeFile.java | 14 +-- .../namenode/snapshot/FileWithSnapshot.java | 17 ++-- .../snapshot/INodeDirectoryWithSnapshot.java | 99 +++++++++---------- .../TestINodeDirectoryWithSnapshot.java | 10 +- 8 files changed, 96 insertions(+), 90 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt index 28bf695151..0e04ec396d 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt +++ b/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) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index 9cea654f6f..9e45f09099 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/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); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java index 4cf8992d7d..96edf12665 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java +++ b/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 pair = recordModification(latest); - INode nodeToUpdate = pair != null ? pair.left : this; + final INode nodeToUpdate = recordModification(latest); 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 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 pair = recordModification(latest); - INode nodeToUpdate = pair != null ? pair.left : this; + final INode nodeToUpdate = recordModification(latest); 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 pair = recordModification(latest); - INode nodeToUpdate = pair != null ? pair.left : this; + final INode nodeToUpdate = recordModification(latest); nodeToUpdate.accessTime = atime; return nodeToUpdate; } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java index 34bc90aaa8..5aa3e67043 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java +++ b/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 recordModification(Snapshot latest) { + public INodeDirectory recordModification(Snapshot latest) { if (latest == null) { - return null; + return this; } - return replaceSelf4INodeDirectoryWithSnapshot(latest) - .save2Snapshot(latest, this); + final INodeDirectoryWithSnapshot withSnapshot + = 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 - * the right inode is the snapshot copy of the child; see also - * {@link INode#createSnapshotCopy()}. + * @return the child inode, which may be replaced. */ - public Pair saveChild2Snapshot( - INode child, Snapshot latest) { + public INode saveChild2Snapshot(INode child, Snapshot latest) { if (latest == null) { - return null; + return child; } return replaceSelf4INodeDirectoryWithSnapshot(latest) .saveChild2Snapshot(child, latest); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java index 04ea98aeaa..cb89eb1817 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java +++ b/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 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 p = recordModification(latest); - if (p != null) { - ((INodeFile)p.left).setFileReplication(replication, null); - return; - } + recordModification(latest).setFileReplication(replication, null); + return; } - header = HeaderFormat.combineReplication(header, replication); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/FileWithSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/FileWithSnapshot.java index ac2895e676..a69aa85949 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/FileWithSnapshot.java +++ b/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 - FileWithSnapshot i = oldFile.getNext(); - newFile.setNext(i); - oldFile.setNext(null); - //find previous element and update it - for(; i.getNext() != oldFile; i = i.getNext()); - i.setNext(newFile); + final FileWithSnapshot oldNext = oldFile.getNext(); + if (oldNext == null) { + newFile.setNext(null); + } else { + if (oldNext != oldFile) { + newFile.setNext(oldNext); + getPrevious(oldFile).setNext(newFile); + } + oldFile.setNext(null); + } } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java index 089b1c8a35..ca4236b9ce 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java +++ b/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 delete(final INode inode, - boolean updateCircularList) { + Triple delete(final INode inode) { 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 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 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 apply2Current(final List 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, - boolean updateCircularList) { + void combinePostDiff(Diff postDiff, Processor deletedINodeProcesser) { final List postCreated = postDiff.created != null? postDiff.created: Collections.emptyList(); final List 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 triple = delete(d, - updateCircularList); + Triple triple = delete(d); 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 triple = modify(d, c, - updateCircularList); + final Triple triple = modify(d, c); 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 checkAndInitINode( - INodeDirectory snapshotCopy) { - if (snapshotINode != null) { - // already initialized. - return null; + private void checkAndInitINode(INodeDirectory snapshotCopy) { + if (snapshotINode == null) { + if (snapshotCopy == null) { + snapshotCopy = new INodeDirectory(INodeDirectoryWithSnapshot.this, + false); + } + snapshotINode = snapshotCopy; } - final INodeDirectoryWithSnapshot dir = INodeDirectoryWithSnapshot.this; - if (snapshotCopy == null) { - snapshotCopy = new INodeDirectory(dir, false); - } - snapshotINode = snapshotCopy; - return new Pair(dir, snapshotCopy); } /** @return the snapshot object of this diff. */ @@ -605,7 +583,7 @@ private List 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 recordModification(Snapshot latest) { - return save2Snapshot(latest, null); + public INodeDirectoryWithSnapshot recordModification(Snapshot latest) { + saveSelf2Snapshot(latest, null); + return this; } /** Save the snapshot copy to the latest snapshot. */ - public Pair save2Snapshot(Snapshot latest, - INodeDirectory snapshotCopy) { - return latest == null? null - : checkAndAddLatestSnapshotDiff(latest).checkAndInitINode(snapshotCopy); + public void saveSelf2Snapshot(Snapshot latest, INodeDirectory snapshotCopy) { + if (latest != null) { + checkAndAddLatestSnapshotDiff(latest).checkAndInitINode(snapshotCopy); + } } @Override - public Pair saveChild2Snapshot( - INode child, Snapshot latest) { + public INode saveChild2Snapshot(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 p = child.createSnapshotCopy(); - diff.diff.modify(p.right, p.left, true); - return p; + if (p.left != p.right) { + final Triple 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 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) { - diff.undoDelete(child, undoInfo); + 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; } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java index 319ce5d2f1..c71e31a4ea 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java +++ b/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 current, Diff diff) { before = toString(diff); } - final Triple undoInfo = diff.delete(inode, true); + final Triple undoInfo = diff.delete(inode); if (testUndo) { final String after = toString(diff); @@ -292,7 +292,7 @@ static void delete(INode inode, final List 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 current, Diff diff) { before = toString(diff); } - final Triple undoInfo = diff.modify(oldinode, newinode, true); + final Triple undoInfo = diff.modify(oldinode, newinode); if (testUndo) { final String after = toString(diff); @@ -322,7 +322,7 @@ static void modify(INode inode, final List 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); } }