Hadoop Common
  1. Hadoop Common
  2. HADOOP-1961

-get, -copyToLocal fail when single filename is passed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.14.1
    • Fix Version/s: 0.14.2, 0.15.0
    • Component/s: None
    • Labels:
      None

      Description

      In 0.14.1 and in trunk, when I try

      % hadoop dfs -get /user/knoguchi/aaa aaa

      get: Failed to rename tmp file to local destination "aaa". Remote source file "/user/knoguchi/aaa" is saved to "/tmp/_copyToLocal_aaa30478".

      This works.

      % hadoop dfs -get /user/knoguchi/aaa ./aaa

      or

      % hadoop dfs -get /user/knoguchi/aaa /home/knoguchi/aaa

      My guess. With change in HADOOP-1292, it now creates a tmp file when -copyToLocal.
      When destination path is passed without any directory, tmp file is created under '/tmp'. Otherwise, it uses the same directory as the destination path.

      In Java API for File.renameTo,
      http://java.sun.com/javase/6/docs/api/java/io/File.html#renameTo(java.io.File)
      it says
      " The rename operation might not be able to move a file from one filesystem to another",

      so renameTo call from /tmp/_tmpfile to /home/knoguchi can fail.

      1. HADOOP-1961-branch14.patch
        5 kB
        Raghu Angadi
      2. HADOOP-1961-branch14.patch
        8 kB
        Raghu Angadi
      3. HADOOP-1961.patch
        2 kB
        Raghu Angadi
      4. HADOOP-1961.patch
        5 kB
        Raghu Angadi
      5. HADOOP-1961.patch
        5 kB
        Raghu Angadi
      6. HADOOP-1961.patch
        8 kB
        Raghu Angadi

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-Nightly #261 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/261/ )
        Hide
        dhruba borthakur added a comment -

        I just committed this. Thanks Raghu!

        Show
        dhruba borthakur added a comment - I just committed this. Thanks Raghu!
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12367089/HADOOP-1961.patch
        against trunk revision r581745.

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

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

        javac +1. The applied patch does not generate any new compiler 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://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/testReport/
        Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/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/12367089/HADOOP-1961.patch against trunk revision r581745. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler 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://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/884/console This message is automatically generated.
        Hide
        Raghu Angadi added a comment -

        I was dealing with multiple patches and totally missed running the unit tests for this.

        TestDFSShell faile because of the following change :

        • bin/hadoop fs -get dir1 localdir # and localdir exists
          • 0.13 and 0.14.1 copy contents of dir1 into localdir
          • 0.14.2 copies dir1 into localdir

        There are no change the fix. Only TestDFSShell.java is changed. Also updated the patch to copy multiple directories.

        Show
        Raghu Angadi added a comment - I was dealing with multiple patches and totally missed running the unit tests for this. TestDFSShell faile because of the following change : bin/hadoop fs -get dir1 localdir # and localdir exists 0.13 and 0.14.1 copy contents of dir1 into localdir 0.14.2 copies dir1 into localdir There are no change the fix. Only TestDFSShell.java is changed. Also updated the patch to copy multiple directories.
        Hide
        Hadoop QA added a comment -

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

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

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

        javac +1. The applied patch does not generate any new compiler 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 failed contrib unit tests.

        Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/testReport/
        Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/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/12367039/HADOOP-1961.patch against trunk revision r581745. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler 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 failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/881/console This message is automatically generated.
        Hide
        Raghu Angadi added a comment -

        > * Should we not use deprecated API anymore? e.g. srcFS.isDirectory(src), srcFS.listPaths(src)

        yeah, I saw that. I just used what ever was there before. I think it will be removed when all the references to it are removed.

        > In the case of rename failing,

        With this fix rename should rarely fail. I think users will be wary of any error. The main concern that Dhruba mentioned is while users are copying very large file and they might might be ok with partial files. Retaining the file only in case of rename failure does not fix that problem.

        Show
        Raghu Angadi added a comment - > * Should we not use deprecated API anymore? e.g. srcFS.isDirectory(src), srcFS.listPaths(src) yeah, I saw that. I just used what ever was there before. I think it will be removed when all the references to it are removed. > In the case of rename failing, With this fix rename should rarely fail. I think users will be wary of any error. The main concern that Dhruba mentioned is while users are copying very large file and they might might be ok with partial files. Retaining the file only in case of rename failure does not fix that problem.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1
        Patch looks good. Below are some minor thoughts.

        • I totally agree that there are redundant copyToLocal methods. See also HADOOP-1544.
        • In the case of rename failing, we know that the tmp file is good but cannot be renamed to dst. User can easily rename the tmp file. For the other exception cases, user don't know what to do to fix the problem. If we remove "deleteOnExit" flag, then we cannot easily tell whether the tmp file is perfect or not.
        • Should we not use deprecated API anymore? e.g. srcFS.isDirectory(src), srcFS.listPaths(src)
        Show
        Tsz Wo Nicholas Sze added a comment - +1 Patch looks good. Below are some minor thoughts. I totally agree that there are redundant copyToLocal methods. See also HADOOP-1544 . In the case of rename failing, we know that the tmp file is good but cannot be renamed to dst. User can easily rename the tmp file. For the other exception cases, user don't know what to do to fix the problem. If we remove "deleteOnExit" flag, then we cannot easily tell whether the tmp file is perfect or not. Should we not use deprecated API anymore? e.g. srcFS.isDirectory(src), srcFS.listPaths(src)
        Hide
        Raghu Angadi added a comment -

        Updated patch for trunk.

        If we want to retain the partially copied file, then fix I would suggest is to remove 'deleteOnExit' flag for the tmp file.

        Show
        Raghu Angadi added a comment - Updated patch for trunk. If we want to retain the partially copied file, then fix I would suggest is to remove 'deleteOnExit' flag for the tmp file.
        Hide
        Raghu Angadi added a comment -

        This patch applies to 0-14 branch. I thought I tested it with trunk as well. I will attach an updated patch for trunk.

        Show
        Raghu Angadi added a comment - This patch applies to 0-14 branch. I thought I tested it with trunk as well. I will attach an updated patch for trunk.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Raghu, your patch cannot be applied to the current trunk. Could you update it?

        Show
        Tsz Wo Nicholas Sze added a comment - Raghu, your patch cannot be applied to the current trunk. Could you update it?
        Hide
        Raghu Angadi added a comment -

        Attached patch keeps copyToLocal() in FsShell.java closer to 0.13 structure.

        Fixes following regressions :

        • copying a file with simple file name for destination as described in the description.
        • handling of globes : globePath() was invoked late and when it returns just one path, treated it as non-globed path, unless the destination was a directory (see example in earlier comment).

        Retains the following change between 0.13 and 0.14 :

        • bin/hadoop fs -get dir1 dir2 localdir # two dirs can be specified with a glob
          • 0.13 copies contents of dir1 and dir2 into localdir
          • 0.14.x copies dir1 and dir2 into localdir (matches with regular cp)

        The following behaviour is new :

        • when rename() fails during the copy does not copy temp file to another temp file.
        • bin/hadoop fs -get dir1 localdir # and localdir exists
          • 0.13 and 0.14.1 copy contents of dir1 into localdir
          • 0.14.2 copies dir1 into localdir
          • btw, when localdir does not exist, all versions copy contents of dir1 into localdir.
        Show
        Raghu Angadi added a comment - Attached patch keeps copyToLocal() in FsShell.java closer to 0.13 structure. Fixes following regressions : copying a file with simple file name for destination as described in the description. handling of globes : globePath() was invoked late and when it returns just one path, treated it as non-globed path, unless the destination was a directory (see example in earlier comment). Retains the following change between 0.13 and 0.14 : bin/hadoop fs -get dir1 dir2 localdir # two dirs can be specified with a glob 0.13 copies contents of dir1 and dir2 into localdir 0.14.x copies dir1 and dir2 into localdir (matches with regular cp) The following behaviour is new : when rename() fails during the copy does not copy temp file to another temp file. bin/hadoop fs -get dir1 localdir # and localdir exists 0.13 and 0.14.1 copy contents of dir1 into localdir 0.14.2 copies dir1 into localdir btw, when localdir does not exist, all versions copy contents of dir1 into localdir.
        Hide
        Raghu Angadi added a comment - - edited

        There are at least three versions of copyToLocal : one in FsShell, one in ChecksumFileSystem, and FileUtil. All of these implement same logic and recursion.. but are slightly different. FsShell and ChecksumFS versions in turn invoke FileUtil version. We should remove FsShell and ChecksumFS, at least in 0.15 or 0.16.

        Show
        Raghu Angadi added a comment - - edited There are at least three versions of copyToLocal : one in FsShell, one in ChecksumFileSystem, and FileUtil. All of these implement same logic and recursion.. but are slightly different. FsShell and ChecksumFS versions in turn invoke FileUtil version. We should remove FsShell and ChecksumFS, at least in 0.15 or 0.16.
        Hide
        Raghu Angadi added a comment -

        There are some more inconsistencies in copyToLocal() :

        For example :

        hadoop-14> bin/hadoop dfs -copyToLocal '/user/rangadi/x*' tmp
        copyToLocal: src "/user/rangadi/x*" does not exist.
        hadoop-14> mkdir tmp
        hadoop-14> bin/hadoop dfs -copyToLocal '/user/rangadi/x*' tmp
        hadoop-14> ls tmp
        x
        

        I will fix this as well. There is also an extra isDirectory() that is not required.

        Show
        Raghu Angadi added a comment - There are some more inconsistencies in copyToLocal() : For example : hadoop-14> bin/hadoop dfs -copyToLocal '/user/rangadi/x*' tmp copyToLocal: src "/user/rangadi/x*" does not exist. hadoop-14> mkdir tmp hadoop-14> bin/hadoop dfs -copyToLocal '/user/rangadi/x*' tmp hadoop-14> ls tmp x I will fix this as well. There is also an extra isDirectory() that is not required.
        Hide
        Raghu Angadi added a comment -

        Fix is to pass dfs.getAbsolutePath() instead of dst for FileUtil.createLocalTempFile().

        Also this removes 'if conditional' around File.copy() since return is expected to be true and not handled when it is false anyway.

        This patch also removes the special treatment of rename(). It is strictly not required for this fix. Should I remove the change?

        Show
        Raghu Angadi added a comment - Fix is to pass dfs.getAbsolutePath() instead of dst for FileUtil.createLocalTempFile() . Also this removes 'if conditional' around File.copy() since return is expected to be true and not handled when it is false anyway. This patch also removes the special treatment of rename(). It is strictly not required for this fix. Should I remove the change?
        Hide
        Raghu Angadi added a comment -

        A comment in HADOOP-1292 says :

        3. I am unable to understand the behaviour of "another" file in FsShell.copyToLocal. Will discuss this with you.

        If renaming tmp to dst failed, tmp is renamed to another file since tmp will be deleted on exit. The error message would tell the user that src is copied to "another" successfully but cannot be renamed to dst.

        I am not sure about the above comment either. When rename() fails wouldn't that be just another case of failure to copy? Once we use same filesystem as the intended destination, rename() won't fail. And if it does fail, we could just treat it as any other failure.

        Show
        Raghu Angadi added a comment - A comment in HADOOP-1292 says : 3. I am unable to understand the behaviour of "another" file in FsShell.copyToLocal. Will discuss this with you. If renaming tmp to dst failed, tmp is renamed to another file since tmp will be deleted on exit. The error message would tell the user that src is copied to "another" successfully but cannot be renamed to dst. I am not sure about the above comment either. When rename() fails wouldn't that be just another case of failure to copy? Once we use same filesystem as the intended destination, rename() won't fail. And if it does fail, we could just treat it as any other failure.

          People

          • Assignee:
            Raghu Angadi
            Reporter:
            Koji Noguchi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development