hi suresh, this seems like a blocker for 0.20 release, isn't it?

changed the priority to blocker

Here is the approach:

1. Current rename implementation removes the src and while moving it to dst, if failure is encountered it adds the src back. This change, before removing src from directory tree, ensures the dst has quota and other conditions needed for rename to succeed is met.
2. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota.
3. Previous version of the code threw QuotaExceededException from FSDirectory.removeChild(). This was unnecessary since the quota was being freed and the exception would never occur. This method no longer throws exception.

2. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota.

What kind of failures might we encounter during the rename? How can we guarantee to have no failures adding src back?

In general, you are trying to make the move operation atomic (either done successfully, or no effect is happening). This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation.

> This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation.
That is what I have captured in bullet 1. All the checks are done prior to removing node. I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block.

I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block.

Are you asserting that restoring src back will guarantee to succeed? (Or die miserably, e.g. OOM and the whole NN will crash).

A typical way of doing this is:

• lock down both src and dest node.
• Create src.shadow and dest.shadow and set up the state of src.shadow and dest.shadow as how src and dest would look like after the rename.
• Support a "guaranteed-to-success" "swap" operation on the node. And "swap" between src.shadow and src, and dest.shadow and dest. This is guaranteed to success.

adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected.

While it was a Good Thing(tm) in this instance, isn't anyone else concerned about the fact that the in memory image and the on disk image are apparently different?

adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected.

Can these conditions be tested with mocks or injection?

The patch looks good. I went through the main logic in unprotectedRenameTo(). Most of conditions in which rename is inapplicable are checked in advance before moving anything anywhere. Everything seems correct. But it will need extensive testing. To make sure all the math is right in verifyQuota(), etc.
Small things:

• Check the value returned by getExistingPathINodes(), compare it with the number of path components.
• verifyQuota() needs JavaDoc - don't want to read code every time I need to know what it does.
• you addChild() with inheritPermission == true. In the old code it was false. Is that intentional?

> will need extensive testing...
Currently TestHDFSCLI, TestQuota and the new test cases added add to a lot of testcases. Let me know if you feel any thing more specific is needed. Also FSDirectory.verifyQuota() is mostly refactored code out of FSDirectory.updateCount().

> Check the value returned by getExistingPathINodes(), compare it with the number of path components.
This is what current checks for dstInodes[dstInodes.length-1] != null and dstInodes[dstInodes.length-2] == null does. If you feel it is still good to add additional check for return value, I can add it.

> you addChild() with inheritPermission == true. In the old code it was false. Is that intentional?
That was not intentional. Thanks for catching it.

Attaching new patch with incorporated comments.

+1.
I see you included a test for renames with quotas.

Latest patch passes all the tests with the following exception:

1. org.apache.hadoop.hdfs.TestFileAppend3 (HDFS-668)
2. org.apache.hadoop.hdfs.server.namenode.TestBackupNode(HDFS-192)

Here is the test-patch result:
[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 16 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

Committed the patch to both 21 and trunk. Will upload and commit the patch for 20 later.

Attaching release 20 version of the patch.

I committed this patch to 20 branch.

Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #47 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/47/)

Integrated in Hadoop-Hdfs-trunk-Commit #79 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/79/)

Integrated in Hadoop-Hdfs-trunk #120 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/120/)

Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #78 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/78/)