You are viewing a plain text version of this content. The canonical link for it is here.
Posted to hdfs-commits@hadoop.apache.org by sz...@apache.org on 2013/01/15 07:20:22 UTC
svn commit: r1433293 - in
/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs: ./
src/main/java/org/apache/hadoop/hdfs/server/namenode/
src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/
src/test/java/org/apache/hadoop/hdfs/se...
Author: szetszwo
Date: Tue Jan 15 06:20:22 2013
New Revision: 1433293
URL: http://svn.apache.org/viewvc?rev=1433293&view=rev
Log:
HDFS-4397. Fix a bug in INodeDirectoryWithSnapshot.Diff.combinePostDiff(..) that it may put the wrong node into the deleted list.
Modified:
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectorySnapshottable.java
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java
hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt Tue Jan 15 06:20:22 2013
@@ -103,3 +103,6 @@ Branch-2802 Snapshot (Unreleased)
HDFS-4395. In INodeDirectorySnapshottable's constructor, the passed-in dir
could be an INodeDirectoryWithSnapshot. (Jing Zhao via szetszwo)
+
+ HDFS-4397. Fix a bug in INodeDirectoryWithSnapshot.Diff.combinePostDiff(..)
+ that it may put the wrong node into the deleted list. (szetszwo)
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java Tue Jan 15 06:20:22 2013
@@ -17,7 +17,6 @@
*/
package org.apache.hadoop.hdfs.server.namenode;
-import java.io.IOException;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.Arrays;
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java Tue Jan 15 06:20:22 2013
@@ -211,7 +211,7 @@ public class INodeFile extends INode imp
}
@Override
- protected int collectSubtreeBlocksAndClear(BlocksMapUpdateInfo info) {
+ public int collectSubtreeBlocksAndClear(BlocksMapUpdateInfo info) {
parent = null;
if(blocks != null && info != null) {
for (BlockInfo blk : blocks) {
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectorySnapshottable.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectorySnapshottable.java?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectorySnapshottable.java (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectorySnapshottable.java Tue Jan 15 06:20:22 2013
@@ -242,7 +242,6 @@ public class INodeDirectorySnapshottable
Snapshot snapshot) {
super.dumpTreeRecursively(out, prefix, snapshot);
- try {
if (snapshot == null) {
out.println();
out.print(prefix);
@@ -296,8 +295,5 @@ public class INodeDirectorySnapshottable
}
});
}
- } catch(Exception e) {
- throw new RuntimeException("this=" + this, e);
- }
}
}
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java Tue Jan 15 06:20:22 2013
@@ -26,6 +26,7 @@ import org.apache.hadoop.fs.permission.F
import org.apache.hadoop.hdfs.server.namenode.INode;
import org.apache.hadoop.hdfs.server.namenode.INodeDirectory;
import org.apache.hadoop.hdfs.server.namenode.INodeDirectoryWithQuota;
+import org.apache.hadoop.hdfs.server.namenode.INodeFile;
import org.apache.hadoop.hdfs.util.ReadOnlyList;
import com.google.common.annotations.VisibleForTesting;
@@ -58,7 +59,7 @@ public class INodeDirectoryWithSnapshot
* 1.1. create i in current: add it to c-list (c, 0)
* 1.1.1. create i in current and then create: impossible
* 1.1.2. create i in current and then delete: remove it from c-list (0, 0)
- * 1.1.3. create i in current and then modify: replace it in c-list (c, 0)
+ * 1.1.3. create i in current and then modify: replace it in c-list (c', 0)
*
* 1.2. delete i from current: impossible
*
@@ -75,7 +76,7 @@ public class INodeDirectoryWithSnapshot
* 2.3. modify i in current: put it in both c-list and d-list (c, d)
* 2.3.1. modify i in current and then create: impossible
* 2.3.2. modify i in current and then delete: remove it from c-list (0, d)
- * 2.3.3. modify i in current and then modify: replace it in c-list (c, d)
+ * 2.3.3. modify i in current and then modify: replace it in c-list (c', d)
* </pre>
*/
static class Diff {
@@ -192,8 +193,11 @@ public class INodeDirectoryWithSnapshot
INode previous = null;
Integer d = null;
if (c >= 0) {
- // Case 1.1.3: inode is already in c-list,
+ // Case 1.1.3 and 2.3.3: inode is already in c-list,
previous = created.set(c, newinode);
+
+ //TODO: fix a bug that previous != oldinode. Set it to oldinode for now
+ previous = oldinode;
} else {
d = search(deleted, oldinode);
if (d < 0) {
@@ -322,38 +326,23 @@ public class INodeDirectoryWithSnapshot
* Note that after this function the postDiff will be deleted.
*
* @param the posterior diff to combine
- * @param collectedBlocks Used in case 2.3, 3.1, and 3.3 to collect
- * information for blocksMap update
+ * @param deletedINodeProcesser Used in case 2.1, 2.3, 3.1, and 3.3
+ * to process the deleted inodes.
*/
- void combinePostDiff(Diff postDiff, BlocksMapUpdateInfo collectedBlocks) {
+ void combinePostDiff(Diff postDiff, Processor deletedINodeProcesser) {
while (postDiff.created != null && !postDiff.created.isEmpty()) {
- INode node = postDiff.created.remove(postDiff.created.size() - 1);
- int deletedIndex = search(postDiff.deleted, node);
+ final INode c = postDiff.created.remove(postDiff.created.size() - 1);
+ final int deletedIndex = search(postDiff.deleted, c);
if (deletedIndex < 0) {
- // for case 1
- create(node);
+ // case 1
+ create(c);
} else {
+ final INode d = postDiff.deleted.remove(deletedIndex);
// case 3
- int createdIndex = search(created, node);
- if (createdIndex < 0) {
- // 3.4
- create(node);
- insertDeleted(node, search(deleted, node));
- } else {
- // 3.1 and 3.3
- created.set(createdIndex, node);
- // for 3.1 and 3.3, if the node is an INodeFileWithLink, need to
- // remove d in the posterior diff from the circular list, also do
- // possible block deletion and blocksMap updating
- INode dInPost = postDiff.deleted.get(deletedIndex);
- if (dInPost instanceof INodeFileWithLink) {
- // dInPost must be an INodeFileWithLink
- ((INodeFileWithLink) dInPost)
- .collectSubtreeBlocksAndClear(collectedBlocks);
- }
+ final Triple<Integer, INode, Integer> triple = modify(d, c);
+ if (deletedINodeProcesser != null) {
+ deletedINodeProcesser.process(triple.middle);
}
- // also remove the inode from the deleted list
- postDiff.deleted.remove(deletedIndex);
}
}
@@ -361,12 +350,8 @@ public class INodeDirectoryWithSnapshot
// case 2
INode node = postDiff.deleted.remove(postDiff.deleted.size() - 1);
Triple<Integer, INode, Integer> triple = delete(node);
- // for 2.3, if the node is an INodeFileWithLink, need to remove c' from
- // the circular list
- INode cInCurrent = triple.middle;
- if (cInCurrent instanceof INodeFileWithLink) {
- ((INodeFileWithLink) cInCurrent)
- .collectSubtreeBlocksAndClear(collectedBlocks);
+ if (deletedINodeProcesser != null) {
+ deletedINodeProcesser.process(triple.middle);
}
}
}
@@ -487,10 +472,12 @@ public class INodeDirectoryWithSnapshot
private List<INode> initChildren() {
if (children == null) {
- final ReadOnlyList<INode> posterior = posteriorDiff != null?
- posteriorDiff.getChildrenList()
- : INodeDirectoryWithSnapshot.this.getChildrenList(null);
- children = diff.apply2Current(ReadOnlyList.Util.asList(posterior));
+ final Diff combined = new Diff();
+ for(SnapshotDiff d = SnapshotDiff.this; d != null; d = d.posteriorDiff) {
+ combined.combinePostDiff(d.diff, null);
+ }
+ children = combined.apply2Current(ReadOnlyList.Util.asList(
+ INodeDirectoryWithSnapshot.this.getChildrenList(null)));
}
return children;
}
@@ -542,6 +529,12 @@ public class INodeDirectoryWithSnapshot
}
}
+ /** An interface for passing a method to process inodes. */
+ static interface Processor {
+ /** Process the given inode. */
+ void process(INode inode);
+ }
+
/** Create an {@link INodeDirectoryWithSnapshot} with the given snapshot.*/
public static INodeDirectoryWithSnapshot newInstance(INodeDirectory dir,
Snapshot latest) {
@@ -576,7 +569,7 @@ public class INodeDirectoryWithSnapshot
* Null if the snapshot with the given name does not exist.
*/
SnapshotDiff deleteSnapshotDiff(Snapshot snapshot,
- BlocksMapUpdateInfo collectedBlocks) {
+ final BlocksMapUpdateInfo collectedBlocks) {
int snapshotIndex = Collections.binarySearch(diffs, snapshot);
if (snapshotIndex == -1) {
return null;
@@ -586,7 +579,16 @@ public class INodeDirectoryWithSnapshot
if (snapshotIndex > 0) {
// combine the to-be-removed diff with its previous diff
SnapshotDiff previousDiff = diffs.get(snapshotIndex - 1);
- previousDiff.diff.combinePostDiff(diffToRemove.diff, collectedBlocks);
+ previousDiff.diff.combinePostDiff(diffToRemove.diff, new Processor() {
+ /** Collect blocks for deleted files. */
+ @Override
+ public void process(INode inode) {
+ if (inode != null && inode instanceof INodeFile) {
+ ((INodeFile)inode).collectSubtreeBlocksAndClear(collectedBlocks);
+ }
+ }
+ });
+
previousDiff.posteriorDiff = diffToRemove.posteriorDiff;
diffToRemove.posteriorDiff = null;
}
Modified: hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java?rev=1433293&r1=1433292&r2=1433293&view=diff
==============================================================================
--- hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java (original)
+++ hadoop/common/branches/HDFS-2802/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java Tue Jan 15 06:20:22 2013
@@ -48,7 +48,7 @@ public class TestINodeDirectoryWithSnaps
public void testDiff() throws Exception {
for(int startSize = 0; startSize <= 1000; startSize = nextStep(startSize)) {
for(int m = 0; m <= 10000; m = nextStep(m)) {
- runDiffTest(startSize, m, true);
+ runDiffTest(startSize, m);
}
}
}
@@ -68,7 +68,7 @@ public class TestINodeDirectoryWithSnaps
* @param numModifications
* @param computeDiff
*/
- void runDiffTest(int startSize, int numModifications, boolean computeDiff) {
+ void runDiffTest(int startSize, int numModifications) {
final int width = findWidth(startSize + numModifications);
System.out.println("\nstartSize=" + startSize
+ ", numModifications=" + numModifications
@@ -83,9 +83,15 @@ public class TestINodeDirectoryWithSnaps
// make modifications to current and record the diff
final List<INode> current = new ArrayList<INode>(previous);
- final Diff diff = computeDiff? new Diff(): null;
+
+ final Diff[] diffs = new Diff[5];
+ for(int j = 0; j < diffs.length; j++) {
+ diffs[j] = new Diff();
+ }
for(int m = 0; m < numModifications; m++) {
+ final int j = m * diffs.length / numModifications;
+
// if current is empty, the next operation must be create;
// otherwise, randomly pick an operation.
final int nextOperation = current.isEmpty()? 1: RANDOM.nextInt(3) + 1;
@@ -93,55 +99,85 @@ public class TestINodeDirectoryWithSnaps
case 1: // create
{
final INode i = newINode(n++, width);
- create(i, current, diff);
+ create(i, current, diffs[j]);
break;
}
case 2: // delete
{
final INode i = current.get(RANDOM.nextInt(current.size()));
- delete(i, current, diff);
+ delete(i, current, diffs[j]);
break;
}
case 3: // modify
{
final INode i = current.get(RANDOM.nextInt(current.size()));
- modify(i, current, diff);
+ modify(i, current, diffs[j]);
break;
}
}
}
- if (computeDiff) {
- // check if current == previous + diff
- final List<INode> c = diff.apply2Previous(previous);
+ {
+ // check if current == previous + diffs
+ List<INode> c = previous;
+ for(int i = 0; i < diffs.length; i++) {
+ c = diffs[i].apply2Previous(c);
+ }
if (!hasIdenticalElements(current, c)) {
System.out.println("previous = " + Diff.toString(previous));
System.out.println();
System.out.println("current = " + Diff.toString(current));
System.out.println("c = " + Diff.toString(c));
- System.out.println();
- System.out.println("diff = " + diff);
throw new AssertionError("current and c are not identical.");
}
- // check if previous == current - diff
- final List<INode> p = diff.apply2Current(current);
+ // check if previous == current - diffs
+ List<INode> p = current;
+ for(int i = diffs.length - 1; i >= 0; i--) {
+ p = diffs[i].apply2Current(p);
+ }
if (!hasIdenticalElements(previous, p)) {
System.out.println("previous = " + Diff.toString(previous));
System.out.println("p = " + Diff.toString(p));
System.out.println();
System.out.println("current = " + Diff.toString(current));
+ throw new AssertionError("previous and p are not identical.");
+ }
+ }
+
+ // combine all diffs
+ final Diff combined = diffs[0];
+ for(int i = 1; i < diffs.length; i++) {
+ combined.combinePostDiff(diffs[i], null);
+ }
+
+ {
+ // check if current == previous + combined
+ final List<INode> c = combined.apply2Previous(previous);
+ if (!hasIdenticalElements(current, c)) {
+ System.out.println("previous = " + Diff.toString(previous));
System.out.println();
- System.out.println("diff = " + diff);
+ System.out.println("current = " + Diff.toString(current));
+ System.out.println("c = " + Diff.toString(c));
+ throw new AssertionError("current and c are not identical.");
+ }
+
+ // check if previous == current - combined
+ final List<INode> p = combined.apply2Current(current);
+ if (!hasIdenticalElements(previous, p)) {
+ System.out.println("previous = " + Diff.toString(previous));
+ System.out.println("p = " + Diff.toString(p));
+ System.out.println();
+ System.out.println("current = " + Diff.toString(current));
throw new AssertionError("previous and p are not identical.");
}
}
- if (computeDiff) {
+ {
for(int m = 0; m < n; m++) {
final INode inode = newINode(m, width);
{// test accessPrevious
- final INode[] array = diff.accessPrevious(inode.getLocalNameBytes());
+ final INode[] array = combined.accessPrevious(inode.getLocalNameBytes());
final INode computed;
if (array != null) {
computed = array[0];
@@ -157,7 +193,7 @@ public class TestINodeDirectoryWithSnaps
}
{// test accessCurrent
- final INode[] array = diff.accessCurrent(inode.getLocalNameBytes());
+ final INode[] array = combined.accessCurrent(inode.getLocalNameBytes());
final INode computed;
if (array != null) {
computed = array[0];
@@ -239,8 +275,6 @@ public class TestINodeDirectoryWithSnaps
static void delete(INode inode, final List<INode> current, Diff diff) {
final int i = Diff.search(current, inode);
- Assert.assertTrue("i=" + i + ", inode=" + inode + "\ncurrent=" + current,
- i >= 0);
current.remove(i);
if (diff != null) {
//test undo with 1/UNDO_TEST_P probability