Return-Path: X-Original-To: apmail-hadoop-hdfs-commits-archive@minotaur.apache.org Delivered-To: apmail-hadoop-hdfs-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 9E9CDD18F for ; Sat, 1 Dec 2012 22:30:18 +0000 (UTC) Received: (qmail 32591 invoked by uid 500); 1 Dec 2012 22:30:18 -0000 Delivered-To: apmail-hadoop-hdfs-commits-archive@hadoop.apache.org Received: (qmail 32553 invoked by uid 500); 1 Dec 2012 22:30:18 -0000 Mailing-List: contact hdfs-commits-help@hadoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: hdfs-dev@hadoop.apache.org Delivered-To: mailing list hdfs-commits@hadoop.apache.org Received: (qmail 32544 invoked by uid 99); 1 Dec 2012 22:30:18 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 01 Dec 2012 22:30:18 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 01 Dec 2012 22:30:16 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 8E7482388980; Sat, 1 Dec 2012 22:29:56 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1416064 - in /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs: ./ src/main/java/org/apache/hadoop/hdfs/server/namenode/ src/test/java/org/apache/hadoop/hdfs/ Date: Sat, 01 Dec 2012 22:29:55 -0000 To: hdfs-commits@hadoop.apache.org From: szetszwo@apache.org X-Mailer: svnmailer-1.0.8-patched Message-Id: <20121201222956.8E7482388980@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: szetszwo Date: Sat Dec 1 22:29:54 2012 New Revision: 1416064 URL: http://svn.apache.org/viewvc?rev=1416064&view=rev Log: HDFS-4248. Renaming directories may incorrectly remove the paths in leases under the tree. Contributed by daryn Modified: 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 Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt Sat Dec 1 22:29:54 2012 @@ -2037,6 +2037,9 @@ Release 0.23.6 - UNRELEASED HDFS-4247. saveNamespace should be tolerant of dangling lease (daryn) + HDFS-4248. Renaming directories may incorrectly remove the paths in leases + under the tree. (daryn via szetszwo) + Release 0.23.5 - UNRELEASED INCOMPATIBLE CHANGES Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java Sat Dec 1 22:29:54 2012 @@ -575,6 +575,8 @@ public class FSDirectory implements Clos // update modification time of dst and the parent of src srcInodes[srcInodes.length-2].setModificationTime(timestamp); dstInodes[dstInodes.length-2].setModificationTime(timestamp); + // update moved leases with new filename + getFSNamesystem().unprotectedChangeLease(src, dst); return true; } } finally { @@ -729,6 +731,8 @@ public class FSDirectory implements Clos } srcInodes[srcInodes.length - 2].setModificationTime(timestamp); dstInodes[dstInodes.length - 2].setModificationTime(timestamp); + // update moved lease with new filename + getFSNamesystem().unprotectedChangeLease(src, dst); // Collect the blocks and remove the lease for previous dst int filesDeleted = 0; Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java Sat Dec 1 22:29:54 2012 @@ -31,7 +31,6 @@ import org.apache.hadoop.classification. import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.hdfs.protocol.Block; import org.apache.hadoop.hdfs.protocol.HdfsConstants; -import org.apache.hadoop.hdfs.protocol.HdfsFileStatus; import org.apache.hadoop.hdfs.protocol.LayoutVersion; import org.apache.hadoop.hdfs.server.blockmanagement.BlockInfo; import org.apache.hadoop.hdfs.server.blockmanagement.BlockInfoUnderConstruction; @@ -360,10 +359,8 @@ public class FSEditLogLoader { } case OP_RENAME_OLD: { RenameOldOp renameOp = (RenameOldOp)op; - HdfsFileStatus dinfo = fsDir.getFileInfo(renameOp.dst, false); fsDir.unprotectedRenameTo(renameOp.src, renameOp.dst, renameOp.timestamp); - fsNamesys.unprotectedChangeLease(renameOp.src, renameOp.dst, dinfo); break; } case OP_DELETE: { @@ -433,11 +430,8 @@ public class FSEditLogLoader { } case OP_RENAME: { RenameOp renameOp = (RenameOp)op; - - HdfsFileStatus dinfo = fsDir.getFileInfo(renameOp.dst, false); fsDir.unprotectedRenameTo(renameOp.src, renameOp.dst, renameOp.timestamp, renameOp.options); - fsNamesys.unprotectedChangeLease(renameOp.src, renameOp.dst, dinfo); break; } case OP_GET_DELEGATION_TOKEN: { Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java Sat Dec 1 22:29:54 2012 @@ -2583,15 +2583,15 @@ public class FSNamesystem implements Nam if (isPermissionEnabled) { //We should not be doing this. This is move() not renameTo(). //but for now, + //NOTE: yes, this is bad! it's assuming much lower level behavior + // of rewriting the dst String actualdst = dir.isDir(dst)? dst + Path.SEPARATOR + new Path(src).getName(): dst; checkParentAccess(src, FsAction.WRITE); checkAncestorAccess(actualdst, FsAction.WRITE); } - HdfsFileStatus dinfo = dir.getFileInfo(dst, false); if (dir.renameTo(src, dst)) { - unprotectedChangeLease(src, dst, dinfo); // update lease with new filename return true; } return false; @@ -2642,9 +2642,7 @@ public class FSNamesystem implements Nam checkAncestorAccess(dst, FsAction.WRITE); } - HdfsFileStatus dinfo = dir.getFileInfo(dst, false); dir.renameTo(src, dst, options); - unprotectedChangeLease(src, dst, dinfo); // update lease with new filename } /** @@ -4885,31 +4883,9 @@ public class FSNamesystem implements Nam // rename was successful. If any part of the renamed subtree had // files that were being written to, update with new filename. - void unprotectedChangeLease(String src, String dst, HdfsFileStatus dinfo) { - String overwrite; - String replaceBy; + void unprotectedChangeLease(String src, String dst) { assert hasWriteLock(); - - boolean destinationExisted = true; - if (dinfo == null) { - destinationExisted = false; - } - - if (destinationExisted && dinfo.isDir()) { - Path spath = new Path(src); - Path parent = spath.getParent(); - if (parent.isRoot()) { - overwrite = parent.toString(); - } else { - overwrite = parent.toString() + Path.SEPARATOR; - } - replaceBy = dst + Path.SEPARATOR; - } else { - overwrite = src; - replaceBy = dst; - } - - leaseManager.changeLease(src, dst, overwrite, replaceBy); + leaseManager.changeLease(src, dst); } /** Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java Sat Dec 1 22:29:54 2012 @@ -331,22 +331,19 @@ public class LeaseManager { } } - synchronized void changeLease(String src, String dst, - String overwrite, String replaceBy) { + synchronized void changeLease(String src, String dst) { if (LOG.isDebugEnabled()) { LOG.debug(getClass().getSimpleName() + ".changelease: " + - " src=" + src + ", dest=" + dst + - ", overwrite=" + overwrite + - ", replaceBy=" + replaceBy); + " src=" + src + ", dest=" + dst); } - final int len = overwrite.length(); + final int len = src.length(); for(Map.Entry entry : findLeaseWithPrefixPath(src, sortedLeasesByPath).entrySet()) { final String oldpath = entry.getKey(); final Lease lease = entry.getValue(); - //overwrite must be a prefix of oldpath - final String newpath = replaceBy + oldpath.substring(len); + // replace stem of src with new destination + final String newpath = dst + oldpath.substring(len); if (LOG.isDebugEnabled()) { LOG.debug("changeLease: replacing " + oldpath + " with " + newpath); } Modified: hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java?rev=1416064&r1=1416063&r2=1416064&view=diff ============================================================================== --- hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java (original) +++ hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLease.java Sat Dec 1 22:29:54 2012 @@ -30,7 +30,9 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Options; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdfs.protocol.ClientProtocol; import org.apache.hadoop.hdfs.server.namenode.NameNodeAdapter; @@ -49,6 +51,10 @@ public class TestLease { ).getLeaseByPath(src.toString()) != null; } + static int leaseCount(MiniDFSCluster cluster) { + return NameNodeAdapter.getLeaseManager(cluster.getNamesystem()).countLease(); + } + static final String dirString = "/test/lease"; final Path dir = new Path(dirString); static final Log LOG = LogFactory.getLog(TestLease.class); @@ -127,6 +133,96 @@ public class TestLease { } @Test + public void testLeaseAfterRename() throws Exception { + MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).numDataNodes(2).build(); + try { + Path p = new Path("/test-file"); + Path d = new Path("/test-d"); + Path d2 = new Path("/test-d-other"); + + // open a file to get a lease + FileSystem fs = cluster.getFileSystem(); + FSDataOutputStream out = fs.create(p); + out.writeBytes("something"); + //out.hsync(); + Assert.assertTrue(hasLease(cluster, p)); + Assert.assertEquals(1, leaseCount(cluster)); + + // just to ensure first fs doesn't have any logic to twiddle leases + DistributedFileSystem fs2 = (DistributedFileSystem) FileSystem.newInstance(fs.getUri(), fs.getConf()); + + // rename the file into an existing dir + LOG.info("DMS: rename file into dir"); + Path pRenamed = new Path(d, p.getName()); + fs2.mkdirs(d); + fs2.rename(p, pRenamed); + Assert.assertFalse(p+" exists", fs2.exists(p)); + Assert.assertTrue(pRenamed+" not found", fs2.exists(pRenamed)); + Assert.assertFalse("has lease for "+p, hasLease(cluster, p)); + Assert.assertTrue("no lease for "+pRenamed, hasLease(cluster, pRenamed)); + Assert.assertEquals(1, leaseCount(cluster)); + + // rename the parent dir to a new non-existent dir + LOG.info("DMS: rename parent dir"); + Path pRenamedAgain = new Path(d2, pRenamed.getName()); + fs2.rename(d, d2); + // src gone + Assert.assertFalse(d+" exists", fs2.exists(d)); + Assert.assertFalse("has lease for "+pRenamed, hasLease(cluster, pRenamed)); + // dst checks + Assert.assertTrue(d2+" not found", fs2.exists(d2)); + Assert.assertTrue(pRenamedAgain+" not found", fs2.exists(pRenamedAgain)); + Assert.assertTrue("no lease for "+pRenamedAgain, hasLease(cluster, pRenamedAgain)); + Assert.assertEquals(1, leaseCount(cluster)); + + // rename the parent dir to existing dir + // NOTE: rename w/o options moves paths into existing dir + LOG.info("DMS: rename parent again"); + pRenamed = pRenamedAgain; + pRenamedAgain = new Path(new Path(d, d2.getName()), p.getName()); + fs2.mkdirs(d); + fs2.rename(d2, d); + // src gone + Assert.assertFalse(d2+" exists", fs2.exists(d2)); + Assert.assertFalse("no lease for "+pRenamed, hasLease(cluster, pRenamed)); + // dst checks + Assert.assertTrue(d+" not found", fs2.exists(d)); + Assert.assertTrue(pRenamedAgain +" not found", fs2.exists(pRenamedAgain)); + Assert.assertTrue("no lease for "+pRenamedAgain, hasLease(cluster, pRenamedAgain)); + Assert.assertEquals(1, leaseCount(cluster)); + + // rename with opts to non-existent dir + pRenamed = pRenamedAgain; + pRenamedAgain = new Path(d2, p.getName()); + fs2.rename(pRenamed.getParent(), d2, Options.Rename.OVERWRITE); + // src gone + Assert.assertFalse(pRenamed.getParent() +" not found", fs2.exists(pRenamed.getParent())); + Assert.assertFalse("has lease for "+pRenamed, hasLease(cluster, pRenamed)); + // dst checks + Assert.assertTrue(d2+" not found", fs2.exists(d2)); + Assert.assertTrue(pRenamedAgain+" not found", fs2.exists(pRenamedAgain)); + Assert.assertTrue("no lease for "+pRenamedAgain, hasLease(cluster, pRenamedAgain)); + Assert.assertEquals(1, leaseCount(cluster)); + + // rename with opts to existing dir + // NOTE: rename with options will not move paths into the existing dir + pRenamed = pRenamedAgain; + pRenamedAgain = new Path(d, p.getName()); + fs2.rename(pRenamed.getParent(), d, Options.Rename.OVERWRITE); + // src gone + Assert.assertFalse(pRenamed.getParent() +" not found", fs2.exists(pRenamed.getParent())); + Assert.assertFalse("has lease for "+pRenamed, hasLease(cluster, pRenamed)); + // dst checks + Assert.assertTrue(d+" not found", fs2.exists(d)); + Assert.assertTrue(pRenamedAgain+" not found", fs2.exists(pRenamedAgain)); + Assert.assertTrue("no lease for "+pRenamedAgain, hasLease(cluster, pRenamedAgain)); + Assert.assertEquals(1, leaseCount(cluster)); + } finally { + cluster.shutdown(); + } + } + + @Test public void testLease() throws Exception { MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).numDataNodes(2).build(); try {