Local file system uses copy to implement rename

• Key: HADOOP-730
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Chris Douglas
• Reporter: Owen O'Malley
• Component/s: fs
• Affects Version/s: 0.8.0
• Fix Version/s: 0.17.0

Description

There is a variable LocalFileSystem.useCopyForRename that is set to true. When true, the local file system will implement rename as a copy followed by a delete. This is likely a performance problem. Is there a reason that useCopyForRename is set?

Comments

[Doug Cutting]

I think the issue was that rename isn't guaranteed to work across volumes. Linux 'mv' typically works across volumes these days, by resorting to copying, but I'm not sure what Java's file rename does.

[Rod Taylor]

Couldn't it try the renameTo() operation and if it fails then fallback to the slower copy&delete method?

if (pathToFile(src).renameTo(pathToFile(dst))) { return true; }
else { return FileUtil.copy(this, src, this, dst, true, getConf()); }

would replace

if (useCopyForRename) { return FileUtil.copy(this, src, this, dst, true, getConf()); } } else return pathToFile(src).renameTo(pathToFile(dst));

[Doug Cutting]

> Couldn't it try the renameTo() operation and if it fails then fallback to the slower copy&delete method?

Sounds good to me. That might provide a nice speedup too. Care to provide a patch?

[Chris Douglas]

This implements Rod's idea and updates LocalFileSystem to reflect its semantics.

[Hadoop QA]

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

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

tests included -1. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.

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

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

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests -1. The patch failed core unit tests.

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

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1767/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1767/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1767/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1767/console

This message is automatically generated.

[Chris Douglas]

Replaced calls to FileSystem::isDirectory() with FileSystem::getFileStatus().isDir() to resolve the deprecation warnings. This doesn't need new unit tests because the existing ones cover the semantics for rename. The core test that failed on Hudson passes on my machine.

[Hadoop QA]

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

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

tests included -1. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.

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

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

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

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

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1768/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1768/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1768/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1768/console

This message is automatically generated.

[Owen O'Malley]

+1 pending a unit test that tests the behavior that was previously broken.

I also think that using if/then/else would be more readable than ?:.

[Chris Douglas]

The rename code was removed from the patch to LocalFileSystem, since ChecksumFileSystem's is sufficient (it was suspected that copying on rename was causing the crc to be regenerated as part of the move, but that was mistaken). Since this doesn't patch a bug, I'm moving it to 0.17. Without the bug fix, it's trivial enough that I don't think it needs a test case. I also changed the ternary operator to an if/else stmt.

[Hadoop QA]

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

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

tests included -1. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.

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

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

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

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

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1794/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1794/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1794/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1794/console

This message is automatically generated.

[Tsz Wo (Nicholas), SZE]

+1 codes look good

[Chris Douglas]

I just committed this.

[Hudson]

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