Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.23.0, 2.0.0-alpha, 3.0.0
    • Fix Version/s: 2.0.3-alpha, 0.23.6
    • Component/s: namenode
    • Labels:
      None

      Description

      Renames of directories may incorrectly remove the paths in leases under the tree being moved.

      1. h4248_20121130_test_rename.patch
        6 kB
        Tsz Wo Nicholas Sze
      2. HDFS-4248.patch
        12 kB
        Daryn Sharp
      3. HDFS-4248.branch-0.23.patch
        12 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1275 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1275/)
          HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1275 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1275/ ) HDFS-4248 . Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1244 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1244/)
          HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1244 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1244/ ) HDFS-4248 . Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #453 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/453/)
          HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416074)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416074
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #453 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/453/ ) HDFS-4248 . Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416074) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416074 Files : /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #54 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/54/)
          HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #54 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/54/ ) HDFS-4248 . Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > As for what happens to the leases after renaming [...] I think namenode should release them

          I agree, I posed this question in the last paragraph of the first comment on this jira. If Nicholas and others think that's the right to do, I can address on another jira since I think this issue is a bit more severe.

          It sounds reasonable to me. I only has a concern that rename may cause a lot of lease recoveries happening at the same time. BTW, I think there is a bug in current code. Let's discuss it in more details in the new JIRA HDFS-4258.

          Show
          Tsz Wo Nicholas Sze added a comment - > As for what happens to the leases after renaming [...] I think namenode should release them I agree, I posed this question in the last paragraph of the first comment on this jira. If Nicholas and others think that's the right to do, I can address on another jira since I think this issue is a bit more severe. It sounds reasonable to me. I only has a concern that rename may cause a lot of lease recoveries happening at the same time. BTW, I think there is a bug in current code. Let's discuss it in more details in the new JIRA HDFS-4258 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Daryn!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Daryn!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3081 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3081/)
          HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3081 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3081/ ) HDFS-4248 . Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn (Revision 1416064) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1416064 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1

          I tried various rename cases and the patch passed all of them.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 I tried various rename cases and the patch passed all of them.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Sure, I will commit the patch once it has passed all the tests.

          Show
          Tsz Wo Nicholas Sze added a comment - Sure, I will commit the patch once it has passed all the tests.
          Hide
          Daryn Sharp added a comment -

          Thanks Nicholas. If you decide it's reasonable, could you please do the commit since my availability this evening will be spotty?

          Show
          Daryn Sharp added a comment - Thanks Nicholas. If you decide it's reasonable, could you please do the commit since my availability this evening will be spotty?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The patch looks good to me. Let's me test it a little bit more.

          Revised summary and description.

          Show
          Tsz Wo Nicholas Sze added a comment - The patch looks good to me. Let's me test it a little bit more. Revised summary and description.
          Hide
          Daryn Sharp added a comment -

          As for what happens to the leases after renaming [...] I think namenode should release them

          I agree, I posed this question in the last paragraph of the first comment on this jira. If Nicholas and others think that's the right to do, I can address on another jira since I think this issue is a bit more severe.

          Does this jira supersede HDFS-2875?

          Yes, I think it will. I filed this jira since it's a bit more than a simple cleanup.

          Show
          Daryn Sharp added a comment - As for what happens to the leases after renaming [...] I think namenode should release them I agree, I posed this question in the last paragraph of the first comment on this jira. If Nicholas and others think that's the right to do, I can address on another jira since I think this issue is a bit more severe. Does this jira supersede HDFS-2875 ? Yes, I think it will. I filed this jira since it's a bit more than a simple cleanup.
          Hide
          Kihwal Lee added a comment -

          Since the leases are used to track all INodeUnderConstruction, two going out of sync is definitely not an acceptable behavior. I think this patch will plug the hole in rename, regardless of the cause, i.e. dropping or failing to update path. So +1 for it.

          As for what happens to the leases after renaming, since the clients are not notifies or there is no "real" inode, the lease becomes useless. I think namenode should release them, through the existing lease release & block recovery mechanism. Even if the old client was writing slowly to a block, they will soon notice it when if the block gets recovered and are unable to update pipeline. This sounds harsh, but can serve as a notification.

          Does this jira supersede HDFS-2875?

          Show
          Kihwal Lee added a comment - Since the leases are used to track all INodeUnderConstruction , two going out of sync is definitely not an acceptable behavior. I think this patch will plug the hole in rename, regardless of the cause, i.e. dropping or failing to update path. So +1 for it. As for what happens to the leases after renaming, since the clients are not notifies or there is no "real" inode, the lease becomes useless. I think namenode should release them, through the existing lease release & block recovery mechanism. Even if the old client was writing slowly to a block, they will soon notice it when if the block gets recovered and are unable to update pipeline. This sounds harsh, but can serve as a notification. Does this jira supersede HDFS-2875 ?
          Hide
          Daryn Sharp added a comment -

          Odd, I was having some renames rewrite paths to nest into the target dir. Maybe I didn't have a clean tree like I thought I did, or maybe it only happened on 23 which is where I did the primary work and then ported to trunk.

          Given that this patch, I think, is a very good cleanup of the existing code and at a minimum prevents removal in some cases instead of a rewrite of leases - which I think causes the dest files under construction to not be written to the image - is it ok to commit this patch?

          Show
          Daryn Sharp added a comment - Odd, I was having some renames rewrite paths to nest into the target dir. Maybe I didn't have a clean tree like I thought I did, or maybe it only happened on 23 which is where I did the primary work and then ported to trunk. Given that this patch, I think, is a very good cleanup of the existing code and at a minimum prevents removal in some cases instead of a rewrite of leases - which I think causes the dest files under construction to not be written to the image - is it ok to commit this patch?
          Hide
          Jing Zhao added a comment -

          I tried to run Daryn's new testcase without his fix, and could get a similar result with Nicholas, i.e., for new rename to existing dir with OVERWRITE opt, the old lease is removed instead of renamed. (This is because in FSDirectory#unprotectedRenameTo(String, String, long, Options.Rename...), FSNamesystem#removePathAndBlocks is called in the end which removes leases with prefix path src. Daryn's patch can fix the bug since the unprotectedChangeLease call is now moved before calling FSNamesystem#removePathAndBlocks.)

          Show
          Jing Zhao added a comment - I tried to run Daryn's new testcase without his fix, and could get a similar result with Nicholas, i.e., for new rename to existing dir with OVERWRITE opt, the old lease is removed instead of renamed. (This is because in FSDirectory#unprotectedRenameTo(String, String, long, Options.Rename...), FSNamesystem#removePathAndBlocks is called in the end which removes leases with prefix path src. Daryn's patch can fix the bug since the unprotectedChangeLease call is now moved before calling FSNamesystem#removePathAndBlocks.)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12555611/h4248_20121130_test_rename.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          -1 javac. The applied patch generated 2014 javac compiler warnings (more than the trunk's current 2013 warnings).

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//artifact/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12555611/h4248_20121130_test_rename.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. -1 javac . The applied patch generated 2014 javac compiler warnings (more than the trunk's current 2013 warnings). +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3584//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h4248_20121130_test_rename.patch: here is the tests I tried. See if you think the tests are correct.

          Show
          Tsz Wo Nicholas Sze added a comment - h4248_20121130_test_rename.patch: here is the tests I tried. See if you think the tests are correct.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hi Daryn,

          I think the bug of dangling lease may not exist after tried some tests, although there is a bug that a lease is removed but not renamed. In my tests, a file is created in src directory but not close, then rename and then check lease. We have 6 cases: whether the old/new rename is used, whether dst exists, and whether overwrite is set for the new API.

          • Case 1) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=true
            Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_790025804_1, pendingcreates: 1]}

            After {}

            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_exist
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_exist/file
            

            FAILED sortedLeases.size() = 0 != 1

          • Case 2) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=false
            Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1062494203_1, pendingcreates: 1]}

            2012-11-30 19:22:48,393 INFO namenode.TestFSDirectory (TestFSDirectory.java:runRename(220)) - Case 2) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=false

            org.apache.hadoop.fs.FileAlreadyExistsException: rename destination /rename_new/dst_exist already exists
            	at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedRenameTo(FSDirectory.java:676)
            	at org.apache.hadoop.hdfs.server.namenode.FSDirectory.renameTo(FSDirectory.java:469)
            	at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.renameToInternal(FSNamesystem.java:2646)
            	at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.renameTo(FSNamesystem.java:2613)
            	...
            

            After

            {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1062494203_1, pendingcreates: 1]}
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_exist
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_new/src
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_new/src/file
            

            SUCCESS

          • Case 3) src=/rename_new/src, dst=/rename_new/dst_nonexist, overwrite=true
            Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1012508552_1, pendingcreates: 1]}

            After

            {/rename_new/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1012508552_1, pendingcreates: 1]}
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_nonexist
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_nonexist/file
            

            SUCCESS

          • Case 4) src=/rename_new/src, dst=/rename_new/dst_nonexist, overwrite=false
            Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1530262891_1, pendingcreates: 1]}

            After

            {/rename_new/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1530262891_1, pendingcreates: 1]}
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_nonexist
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_new/dst_nonexist/file
            

            SUCCESS

          • Case 5) src=/rename_depreacted/src, dst=/rename_depreacted/dst_exist, overwrite=true
            Before {/rename_depreacted/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_925124308_1, pendingcreates: 1]}

            After

            {/rename_depreacted/dst_exist/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_925124308_1, pendingcreates: 1]}
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_depreacted/dst_exist
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_depreacted/dst_exist/src
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_depreacted/dst_exist/src/file
            

            SUCCESS

          • Case 6) src=/rename_depreacted/src, dst=/rename_depreacted/dst_nonexist, overwrite=true
            Before {/rename_depreacted/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1211709026_1, pendingcreates: 1]}

            After

            {/rename_depreacted/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1211709026_1, pendingcreates: 1]}
            drwxr-xr-x   - szetszwo supergroup          0 2012-11-30 19:22 /rename_depreacted/dst_nonexist
            -rw-r--r--   3 szetszwo supergroup          0 2012-11-30 19:22 /rename_depreacted/dst_nonexist/file
            

            SUCCESS

          Only Case 1 fails but it does not generate a dangling lease.

          Show
          Tsz Wo Nicholas Sze added a comment - Hi Daryn, I think the bug of dangling lease may not exist after tried some tests, although there is a bug that a lease is removed but not renamed. In my tests, a file is created in src directory but not close, then rename and then check lease. We have 6 cases: whether the old/new rename is used, whether dst exists, and whether overwrite is set for the new API. Case 1) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=true Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_790025804_1, pendingcreates: 1]} After {} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_exist -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_exist/file FAILED sortedLeases.size() = 0 != 1 Case 2) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=false Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1062494203_1, pendingcreates: 1]} 2012-11-30 19:22:48,393 INFO namenode.TestFSDirectory (TestFSDirectory.java:runRename(220)) - Case 2) src=/rename_new/src, dst=/rename_new/dst_exist, overwrite=false org.apache.hadoop.fs.FileAlreadyExistsException: rename destination /rename_new/dst_exist already exists at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedRenameTo(FSDirectory.java:676) at org.apache.hadoop.hdfs.server.namenode.FSDirectory.renameTo(FSDirectory.java:469) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.renameToInternal(FSNamesystem.java:2646) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.renameTo(FSNamesystem.java:2613) ... After {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1062494203_1, pendingcreates: 1]} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_exist drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_new/src -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_new/src/file SUCCESS Case 3) src=/rename_new/src, dst=/rename_new/dst_nonexist, overwrite=true Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1012508552_1, pendingcreates: 1]} After {/rename_new/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1012508552_1, pendingcreates: 1]} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_nonexist -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_nonexist/file SUCCESS Case 4) src=/rename_new/src, dst=/rename_new/dst_nonexist, overwrite=false Before {/rename_new/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1530262891_1, pendingcreates: 1]} After {/rename_new/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1530262891_1, pendingcreates: 1]} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_nonexist -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_new/dst_nonexist/file SUCCESS Case 5) src=/rename_depreacted/src, dst=/rename_depreacted/dst_exist, overwrite=true Before {/rename_depreacted/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_925124308_1, pendingcreates: 1]} After {/rename_depreacted/dst_exist/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_925124308_1, pendingcreates: 1]} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_depreacted/dst_exist drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_depreacted/dst_exist/src -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_depreacted/dst_exist/src/file SUCCESS Case 6) src=/rename_depreacted/src, dst=/rename_depreacted/dst_nonexist, overwrite=true Before {/rename_depreacted/src/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1211709026_1, pendingcreates: 1]} After {/rename_depreacted/dst_nonexist/file=[Lease. Holder: DFSClient_NONMAPREDUCE_-1211709026_1, pendingcreates: 1]} drwxr-xr-x - szetszwo supergroup 0 2012-11-30 19:22 /rename_depreacted/dst_nonexist -rw-r--r-- 3 szetszwo supergroup 0 2012-11-30 19:22 /rename_depreacted/dst_nonexist/file SUCCESS Only Case 1 fails but it does not generate a dangling lease.
          Hide
          Jing Zhao added a comment -

          The patch looks very good. +1 for the patch. The only very minor issue is that for the new testcase testLeaseAfterRename() we can remove the commented "out.hsync();" line.

          Show
          Jing Zhao added a comment - The patch looks very good. +1 for the patch. The only very minor issue is that for the new testcase testLeaseAfterRename() we can remove the commented "out.hsync();" line.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12555586/HDFS-4248.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3583//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3583//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12555586/HDFS-4248.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3583//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3583//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          I should mention that pushing the change lease calls lower in the stack allowed the real path to be passed. Ie. no more guessing if the src was nested in the dest. The one used in the rename op is the exact same one used to rewrite the leases.

          Show
          Daryn Sharp added a comment - I should mention that pushing the change lease calls lower in the stack allowed the real path to be passed. Ie. no more guessing if the src was nested in the dest. The one used in the rename op is the exact same one used to rewrite the leases.
          Hide
          Daryn Sharp added a comment -

          The code for renaming and leasing handling is problematic. There's two parallel code paths for rename, based on whether you call it with src/dest or src/dest/opts. They are generally the same copy-n-paste except if the target is a pre-existing directory: the src/dest method will implicitly nested the src under the dest, ie. rename /d1 to /d2 produces /d1/d2. The src/dest/opts method will fail unless the OVERWRITE option is used - which will replace the dest, ie. rename /d1 to /d2 will overwrite /d2.

          The logic for the replacing or nest into the target is so low level that the callers don't know the dest was changed. FSEditLogLoader and FSNameSystem both had to pass a file status of the dest to change the lease so it could "assume" if the dest was to be nested. Unfortunately this causes the src/dest/opt method to have leases incorrectly rewritten, ie. renaming /d1 to /d2 with overwrite produced leases of /d2/d1/d1-contents while the fs had /d2/d1-contents.

          As best I can tell, the saving of the namespace is disjointed for finalized inodes, and underconstruction inodes. First the finalized inodes are serialized. Then the path strings in the leases are used to locate underconstruction inodes for serialization. This seems a bit fragile, in that data loss will occur if the lease path goes out of sync with the namespace paths. I believe with this design, the state of the leases are so integral to the integrity of the namespace, that updates to the namespace and leases much occur "atomically" under the same lock at the same time.

          Also, the src/dest/opts method appeared to just "throw away" the src leases resulting in data loss for open files.

          So... What I did:

          • changing lease paths only requires src & dest, no more file status of the dest before the operation
          • the namespace and the edit log loader aren't both responsible for the call to update lease paths
          • the leases are updated immediately after the move is successfully
          • the src/dest/opts method is not allowed to throw away the source leases

          I'm thinking another jira is needed to immediately reclaim leases on src files. Otherwise files being written by one user can be moved by another. The updated lease ensures image integrity, but is useless to the writer because it fails NN operations (path-based) and has no way to know where the file went and it can't close the file either because the original path no longer exists and leases are looked up based on path. The client continues to hold the lease until it stops actively writing to other streams, which for a daemon might be a really long time.

          Show
          Daryn Sharp added a comment - The code for renaming and leasing handling is problematic. There's two parallel code paths for rename, based on whether you call it with src/dest or src/dest/opts. They are generally the same copy-n-paste except if the target is a pre-existing directory: the src/dest method will implicitly nested the src under the dest, ie. rename /d1 to /d2 produces /d1/d2. The src/dest/opts method will fail unless the OVERWRITE option is used - which will replace the dest, ie. rename /d1 to /d2 will overwrite /d2. The logic for the replacing or nest into the target is so low level that the callers don't know the dest was changed. FSEditLogLoader and FSNameSystem both had to pass a file status of the dest to change the lease so it could "assume" if the dest was to be nested. Unfortunately this causes the src/dest/opt method to have leases incorrectly rewritten, ie. renaming /d1 to /d2 with overwrite produced leases of /d2/d1/d1-contents while the fs had /d2/d1-contents. As best I can tell, the saving of the namespace is disjointed for finalized inodes, and underconstruction inodes. First the finalized inodes are serialized. Then the path strings in the leases are used to locate underconstruction inodes for serialization. This seems a bit fragile, in that data loss will occur if the lease path goes out of sync with the namespace paths. I believe with this design, the state of the leases are so integral to the integrity of the namespace, that updates to the namespace and leases much occur "atomically" under the same lock at the same time. Also, the src/dest/opts method appeared to just "throw away" the src leases resulting in data loss for open files. So... What I did: changing lease paths only requires src & dest, no more file status of the dest before the operation the namespace and the edit log loader aren't both responsible for the call to update lease paths the leases are updated immediately after the move is successfully the src/dest/opts method is not allowed to throw away the source leases I'm thinking another jira is needed to immediately reclaim leases on src files. Otherwise files being written by one user can be moved by another. The updated lease ensures image integrity, but is useless to the writer because it fails NN operations (path-based) and has no way to know where the file went and it can't close the file either because the original path no longer exists and leases are looked up based on path. The client continues to hold the lease until it stops actively writing to other streams, which for a daemon might be a really long time.

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development