HADOOP-9805. Refactor RawLocalFileSystem#rename for improved testability. Contributed by Jean-Pierre Matsumoto.
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/503e4902 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/503e4902 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/503e4902 Branch: refs/heads/YARN-2928 Commit: 503e4902c88cbb6ddcfb423e8530c431dcdf0a8f Parents: 88c1468 Author: cnauroth <[email protected]> Authored: Thu Apr 2 16:13:00 2015 -0700 Committer: Zhijie Shen <[email protected]> Committed: Mon Apr 6 12:08:14 2015 -0700 ---------------------------------------------------------------------- hadoop-common-project/hadoop-common/CHANGES.txt | 3 ++ .../apache/hadoop/fs/RawLocalFileSystem.java | 36 ++++++++++----- .../rawlocal/TestRawlocalContractRename.java | 47 ++++++++++++++++++++ 3 files changed, 74 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/503e4902/hadoop-common-project/hadoop-common/CHANGES.txt ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index c92e378..260cc83 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -476,6 +476,9 @@ Release 2.8.0 - UNRELEASED HADOOP-11660. Add support for hardware crc of HDFS checksums on ARM aarch64 architecture (Edward Nevill via Colin P. McCabe) + HADOOP-9805. Refactor RawLocalFileSystem#rename for improved testability. + (Jean-Pierre Matsumoto via cnauroth) + OPTIMIZATIONS BUG FIXES http://git-wip-us.apache.org/repos/asf/hadoop/blob/503e4902/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java index d7866b8..52623b8 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java @@ -43,7 +43,6 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.io.IOUtils; import org.apache.hadoop.io.nativeio.NativeIO; -import org.apache.hadoop.io.nativeio.NativeIOException; import org.apache.hadoop.util.Progressable; import org.apache.hadoop.util.Shell; import org.apache.hadoop.util.StringUtils; @@ -347,11 +346,29 @@ public class RawLocalFileSystem extends FileSystem { return true; } - // Enforce POSIX rename behavior that a source directory replaces an existing - // destination if the destination is an empty directory. On most platforms, - // this is already handled by the Java API call above. Some platforms - // (notably Windows) do not provide this behavior, so the Java API call above - // fails. Delete destination and attempt rename again. + // Else try POSIX style rename on Windows only + if (Shell.WINDOWS && + handleEmptyDstDirectoryOnWindows(src, srcFile, dst, dstFile)) { + return true; + } + + // The fallback behavior accomplishes the rename by a full copy. + if (LOG.isDebugEnabled()) { + LOG.debug("Falling through to a copy of " + src + " to " + dst); + } + return FileUtil.copy(this, src, this, dst, true, getConf()); + } + + @VisibleForTesting + public final boolean handleEmptyDstDirectoryOnWindows(Path src, File srcFile, + Path dst, File dstFile) throws IOException { + + // Enforce POSIX rename behavior that a source directory replaces an + // existing destination if the destination is an empty directory. On most + // platforms, this is already handled by the Java API call above. Some + // platforms (notably Windows) do not provide this behavior, so the Java API + // call renameTo(dstFile) fails. Delete destination and attempt rename + // again. if (this.exists(dst)) { FileStatus sdst = this.getFileStatus(dst); if (sdst.isDirectory() && dstFile.list().length == 0) { @@ -364,12 +381,7 @@ public class RawLocalFileSystem extends FileSystem { } } } - - // The fallback behavior accomplishes the rename by a full copy. - if (LOG.isDebugEnabled()) { - LOG.debug("Falling through to a copy of " + src + " to " + dst); - } - return FileUtil.copy(this, src, this, dst, true, getConf()); + return false; } @Override http://git-wip-us.apache.org/repos/asf/hadoop/blob/503e4902/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/rawlocal/TestRawlocalContractRename.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/rawlocal/TestRawlocalContractRename.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/rawlocal/TestRawlocalContractRename.java index 9e33b34..25611f1 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/rawlocal/TestRawlocalContractRename.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/rawlocal/TestRawlocalContractRename.java @@ -19,8 +19,13 @@ package org.apache.hadoop.fs.contract.rawlocal; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.RawLocalFileSystem; import org.apache.hadoop.fs.contract.AbstractContractRenameTest; import org.apache.hadoop.fs.contract.AbstractFSContract; +import org.apache.hadoop.fs.contract.ContractTestUtils; +import org.junit.Test; public class TestRawlocalContractRename extends AbstractContractRenameTest { @@ -28,5 +33,47 @@ public class TestRawlocalContractRename extends AbstractContractRenameTest { protected AbstractFSContract createContract(Configuration conf) { return new RawlocalFSContract(conf); } + + /** + * Test fallback rename code <code>handleEmptyDstDirectoryOnWindows()</code> + * even on not Windows platform where the normal <code>File.renameTo()</code> + * is supposed to work well. This test has been added for HADOOP-9805. + * + * @see AbstractContractRenameTest#testRenameWithNonEmptySubDirPOSIX() + */ + @Test + public void testRenameWithNonEmptySubDirPOSIX() throws Throwable { + final Path renameTestDir = path("testRenameWithNonEmptySubDir"); + final Path srcDir = new Path(renameTestDir, "src1"); + final Path srcSubDir = new Path(srcDir, "sub"); + final Path finalDir = new Path(renameTestDir, "dest"); + FileSystem fs = getFileSystem(); + ContractTestUtils.rm(fs, renameTestDir, true, false); + + fs.mkdirs(srcDir); + fs.mkdirs(finalDir); + ContractTestUtils.writeTextFile(fs, new Path(srcDir, "source.txt"), + "this is the file in src dir", false); + ContractTestUtils.writeTextFile(fs, new Path(srcSubDir, "subfile.txt"), + "this is the file in src/sub dir", false); + + ContractTestUtils.assertPathExists(fs, "not created in src dir", + new Path(srcDir, "source.txt")); + ContractTestUtils.assertPathExists(fs, "not created in src/sub dir", + new Path(srcSubDir, "subfile.txt")); + + RawLocalFileSystem rlfs = (RawLocalFileSystem) fs; + rlfs.handleEmptyDstDirectoryOnWindows(srcDir, rlfs.pathToFile(srcDir), + finalDir, rlfs.pathToFile(finalDir)); + + // Accept only POSIX rename behavior in this test + ContractTestUtils.assertPathExists(fs, "not renamed into dest dir", + new Path(finalDir, "source.txt")); + ContractTestUtils.assertPathExists(fs, "not renamed into dest/sub dir", + new Path(finalDir, "sub/subfile.txt")); + + ContractTestUtils.assertPathDoesNotExist(fs, "not deleted", + new Path(srcDir, "source.txt")); + } }
