You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-commits@hadoop.apache.org by cn...@apache.org on 2013/07/26 23:16:17 UTC
svn commit: r1507447 - in /hadoop/common/branches/branch-1-win:
CHANGES.branch-1-win.txt
src/core/org/apache/hadoop/fs/RawLocalFileSystem.java
src/test/org/apache/hadoop/fs/TestLocalFileSystem.java
Author: cnauroth
Date: Fri Jul 26 21:16:17 2013
New Revision: 1507447
URL: http://svn.apache.org/r1507447
Log:
HADOOP-9507. LocalFileSystem rename() is broken in some cases when destination exists. Contributed by Chris Nauroth.
Modified:
hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt
hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/fs/RawLocalFileSystem.java
hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/fs/TestLocalFileSystem.java
Modified: hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt?rev=1507447&r1=1507446&r2=1507447&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt (original)
+++ hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt Fri Jul 26 21:16:17 2013
@@ -469,3 +469,6 @@ Branch-hadoop-1-win (branched from branc
MAPREDUCE-5405. Job recovery can fail if task log directory symlink from
prior run still exists. (cnauroth)
+
+ HADOOP-9507. LocalFileSystem rename() is broken in some cases when
+ destination exists. (cnauroth)
Modified: hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/fs/RawLocalFileSystem.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/fs/RawLocalFileSystem.java?rev=1507447&r1=1507446&r2=1507447&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/fs/RawLocalFileSystem.java (original)
+++ hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/fs/RawLocalFileSystem.java Fri Jul 26 21:16:17 2013
@@ -285,37 +285,36 @@ public class RawLocalFileSystem extends
}
public boolean rename(Path src, Path dst) throws IOException {
- if (pathToFile(src).renameTo(pathToFile(dst))) {
+ // Attempt rename using Java API.
+ File srcFile = pathToFile(src);
+ File dstFile = pathToFile(dst);
+ if (srcFile.renameTo(dstFile)) {
return true;
}
- LOG.debug("Falling through to a copy of " + src + " to " + dst);
-
- // TODO: What if src and dst are same or subset of another?
+ // 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.
if (this.exists(dst)) {
FileStatus sdst = this.getFileStatus(dst);
- if (sdst.isDir()) {
- // If dst exists and is a folder, we have to copy the source content, otherwise
- // we will copy the src folder into the dst folder what is not the desired
- // behavior for rename
-
- FileStatus contents[] = this.listStatus(src);
- for (int i = 0; i < contents.length; i++) {
- FileUtil.copy(this, contents[i].getPath(), this,
- new Path(dst, contents[i].getPath().getName()),
- true, getConf());
+ if (sdst.isDir() && dstFile.list().length == 0) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Deleting empty destination and renaming " + src + " to " +
+ dst);
+ }
+ if (this.delete(dst, false) && srcFile.renameTo(dstFile)) {
+ return true;
}
-
- // Delete the source folder
- return this.delete(src, true);
- }
- else {
- return FileUtil.copy(this, src, this, dst, true, getConf());
}
}
- else {
- return FileUtil.copy(this, src, this, dst, true, getConf());
+
+ // 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());
}
@Deprecated
Modified: hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/fs/TestLocalFileSystem.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/fs/TestLocalFileSystem.java?rev=1507447&r1=1507446&r2=1507447&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/fs/TestLocalFileSystem.java (original)
+++ hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/fs/TestLocalFileSystem.java Fri Jul 26 21:16:17 2013
@@ -18,6 +18,7 @@
package org.apache.hadoop.fs;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.io.IOUtils;
import java.io.*;
import junit.framework.*;
@@ -203,4 +204,125 @@ public class TestLocalFileSystem extends
new RawLocalFileSystem().new LocalFSFileInputStream(path), 1024);
assertNotNull(bis.getFileDescriptor());
}
+
+ /**
+ * Tests a simple rename of a directory.
+ */
+ public void testRenameDirectory() throws IOException {
+ FileSystem fs = FileSystem.getLocal(new Configuration());
+ Path src = new Path(TEST_ROOT_DIR, "dir1");
+ Path dst = new Path(TEST_ROOT_DIR, "dir2");
+ try {
+ fs.delete(src, true);
+ fs.delete(dst, true);
+ assertTrue(fs.mkdirs(src));
+ assertTrue(fs.rename(src, dst));
+ assertTrue(fs.exists(dst));
+ assertFalse(fs.exists(src));
+ } finally {
+ cleanupFileSystem(fs, src, dst);
+ }
+ }
+
+ /**
+ * Tests that renaming a directory replaces the destination if the destination
+ * is an existing empty directory.
+ *
+ * Before:
+ * /dir1
+ * /file1
+ * /file2
+ * /dir2
+ *
+ * After rename("/dir1", "/dir2"):
+ * /dir2
+ * /file1
+ * /file2
+ */
+ public void testRenameReplaceExistingEmptyDirectory() throws IOException {
+ FileSystem fs = FileSystem.getLocal(new Configuration());
+ Path src = new Path(TEST_ROOT_DIR, "dir1");
+ Path dst = new Path(TEST_ROOT_DIR, "dir2");
+ try {
+ fs.delete(src, true);
+ fs.delete(dst, true);
+ assertTrue(fs.mkdirs(src));
+ writeFile(fs, new Path(src, "file1"));
+ writeFile(fs, new Path(src, "file2"));
+ assertTrue(fs.mkdirs(dst));
+ assertTrue(fs.rename(src, dst));
+ assertTrue(fs.exists(dst));
+ assertTrue(fs.exists(new Path(dst, "file1")));
+ assertTrue(fs.exists(new Path(dst, "file2")));
+ assertFalse(fs.exists(src));
+ } finally {
+ cleanupFileSystem(fs, src, dst);
+ }
+ }
+
+ /**
+ * Tests that renaming a directory to an existing directory that is not empty
+ * results in a full copy of source to destination.
+ *
+ * Before:
+ * /dir1
+ * /dir2
+ * /dir3
+ * /file1
+ * /file2
+ *
+ * After rename("/dir1/dir2/dir3", "/dir1"):
+ * /dir1
+ * /dir3
+ * /file1
+ * /file2
+ */
+ public void testRenameMoveToExistingNonEmptyDirectory() throws IOException {
+ FileSystem fs = FileSystem.getLocal(new Configuration());
+ Path src = new Path(TEST_ROOT_DIR, "dir1/dir2/dir3");
+ Path dst = new Path(TEST_ROOT_DIR, "dir1");
+ try {
+ fs.delete(src, true);
+ fs.delete(dst, true);
+ assertTrue(fs.mkdirs(src));
+ writeFile(fs, new Path(src, "file1"));
+ writeFile(fs, new Path(src, "file2"));
+ assertTrue(fs.exists(dst));
+ assertTrue(fs.rename(src, dst));
+ assertTrue(fs.exists(dst));
+ assertTrue(fs.exists(new Path(dst, "dir3")));
+ assertTrue(fs.exists(new Path(dst, "dir3/file1")));
+ assertTrue(fs.exists(new Path(dst, "dir3/file2")));
+ assertFalse(fs.exists(src));
+ } finally {
+ cleanupFileSystem(fs, src, dst);
+ }
+ }
+
+ /**
+ * Cleans up the file system by deleting the given paths and closing the file
+ * system.
+ *
+ * @param fs FileSystem to clean up
+ * @param paths Path... any number of paths to delete
+ */
+ private static void cleanupFileSystem(FileSystem fs, Path... paths) {
+ for (Path path: paths) {
+ deleteQuietly(fs, path);
+ }
+ IOUtils.cleanup(null, fs);
+ }
+
+ /**
+ * Deletes the given path and silences any exceptions.
+ *
+ * @param fs FileSystem to perform the delete
+ * @param path Path to delete
+ */
+ private static void deleteQuietly(FileSystem fs, Path path) {
+ try {
+ fs.delete(path, true);
+ } catch (IOException e) {
+ }
+ }
}