Hadoop Common
  1. Hadoop Common
  2. HADOOP-6563

Add more tests to FileContextSymlinkBaseTest that cover intermediate symlinks in paths

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: fs, test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Intermediate symlinks in paths are covered by the tests that use /linkToDir/file, /linkToDir/subDir, etc eg testCreateVia* in FileContextSymlinkBaseTest. I'll add additional tests to cover other basic operations on paths like /dir/linkToSomeDir/file beyond create() and open().

      1. hadoop-6563-5.patch
        25 kB
        Eli Collins
      2. hadoop-6563-4.patch
        26 kB
        Eli Collins
      3. hadoop-6563-3.patch
        27 kB
        Eli Collins
      4. hadoop-6563-1.patch
        32 kB
        Eli Collins
      5. ASF.LICENSE.NOT.GRANTED--hadoop-6563-2.patch
        32 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Attached patch adds test coverage for intermediate symlinks in paths, and fixes a bug these tests uncovered.

          It also adds a lot of rename unit tests so that the full set of behavior with symlinks, and it conforms to the posix behavior (modulo the overwrite option which is not present in rename(2)). Here's the current FileContext#rename behavior, renaming source to dest on HDFS, augmented with symlinks:

          • If source does not exist then a FileNotFoundException is thrown.
          • If dest does not exist then source is renamed dest. If source is a symlink then the symlink itself is renamed, not the link target.
          • If dest exists and is a non-empty directory then an IOException is thrown.
          • If dest exists and the OVERWRITE option is not given then an IOException is thrown.
          • If dest exists and the OVERWRITE option is given then the behavior depends on the type of source and dest:
          Source Dest Result
          fileA fileB Renames fileA to fileB
            dirB Fails: both need to be file or directory
            linkB fileA is renamed linkB (regardless of what linkB points to)
          dirA fileB Fails: both need to be file or directory
            dirB Renames dirA to dirB (dirB is empty)
            linkB Fails: both need to be file or directory
          linkA fileB Renames linkA to fileB
            dirB Fails: dirB is a directory
            linkB Renames linkA to linkB (regardless of what linkB points to)
          Show
          Eli Collins added a comment - Attached patch adds test coverage for intermediate symlinks in paths, and fixes a bug these tests uncovered. It also adds a lot of rename unit tests so that the full set of behavior with symlinks, and it conforms to the posix behavior (modulo the overwrite option which is not present in rename(2) ). Here's the current FileContext#rename behavior, renaming source to dest on HDFS, augmented with symlinks: If source does not exist then a FileNotFoundException is thrown. If dest does not exist then source is renamed dest . If source is a symlink then the symlink itself is renamed, not the link target. If dest exists and is a non-empty directory then an IOException is thrown. If dest exists and the OVERWRITE option is not given then an IOException is thrown. If dest exists and the OVERWRITE option is given then the behavior depends on the type of source and dest : Source Dest Result fileA fileB Renames fileA to fileB   dirB Fails: both need to be file or directory   linkB fileA is renamed linkB (regardless of what linkB points to) dirA fileB Fails: both need to be file or directory   dirB Renames dirA to dirB ( dirB is empty)   linkB Fails: both need to be file or directory linkA fileB Renames linkA to fileB   dirB Fails: dirB is a directory   linkB Renames linkA to linkB (regardless of what linkB points to)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12441102/hadoop-6563-1.patch
          against trunk revision 931226.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/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/12441102/hadoop-6563-1.patch against trunk revision 931226. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/447/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Fixes the javadoc warning.

          Show
          Eli Collins added a comment - Fixes the javadoc warning.
          Hide
          Suresh Srinivas added a comment -

          Eli, does this functionality comply with rename specification of posix? It is available at http://www.opengroup.org/onlinepubs/000095399/functions/rename.html.

          Show
          Suresh Srinivas added a comment - Eli, does this functionality comply with rename specification of posix? It is available at http://www.opengroup.org/onlinepubs/000095399/functions/rename.html .
          Hide
          Eli Collins added a comment -

          Yes. See the initial comment for the caveat "conforms to the posix behavior (modulo the overwrite option which is not present in rename(2))".

          Show
          Eli Collins added a comment - Yes. See the initial comment for the caveat "conforms to the posix behavior (modulo the overwrite option which is not present in rename(2))".
          Hide
          Eli Collins added a comment -

          Kicking hudson.

          Show
          Eli Collins added a comment - Kicking hudson.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12441659/hadoop-6563-2.patch
          against trunk revision 938590.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/484/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/12441659/hadoop-6563-2.patch against trunk revision 938590. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/484/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Attached patch merges with trunk.

          Show
          Eli Collins added a comment - Attached patch merges with trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12443020/hadoop-6563-3.patch
          against trunk revision 938590.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

          -1 javac. The patch appears to cause tar ant target to fail.

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

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/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/12443020/hadoop-6563-3.patch against trunk revision 938590. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/57/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Right patch this time - sorry for the noise.

          Show
          Eli Collins added a comment - Right patch this time - sorry for the noise.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12443025/hadoop-6563-4.patch
          against trunk revision 938590.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/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/12443025/hadoop-6563-4.patch against trunk revision 938590. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/59/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Modified the table you have a little bit. Hopefully it has more clarity:

          Source Dest Result
          fileA fileB fileB is removed. fileA is renamed fileB
            dirB Fails: both need to be file or directory
            linkB linkB is removed (regardles of what it points to). fileA is renamed linkB
          dirA fileB Fails: both need to be file or directory
            dirB dirB is deleted(if empty). dirA is renamed to dirB
            linkB Fails: if Source is a directory, then Dest must be a directory
          linkA fileB fileB is deleted. linkA renamed to fileB
            dirB if Dest is a directory then Source must be a directory
            linkB linkB is removed (regardless of what it points to. linkA is renamed to linkB

          Comments:

          1. typo danling
          2. testRenameSymlinkToFileItLinksTo - when rename fails, should the test check for exception? Is this part of another jira?
          3. Why is creating dangling links allowed?
          4. Tests are hard to follow without additional descprition in the test methods what is being tested and what is expected. It took me a while to follow the tests.
          Show
          Suresh Srinivas added a comment - Modified the table you have a little bit. Hopefully it has more clarity: Source Dest Result fileA fileB fileB is removed. fileA is renamed fileB   dirB Fails: both need to be file or directory   linkB linkB is removed (regardles of what it points to). fileA is renamed linkB dirA fileB Fails: both need to be file or directory   dirB dirB is deleted(if empty). dirA is renamed to dirB   linkB Fails: if Source is a directory, then Dest must be a directory linkA fileB fileB is deleted. linkA renamed to fileB   dirB if Dest is a directory then Source must be a directory   linkB linkB is removed (regardless of what it points to. linkA is renamed to linkB Comments: typo danling testRenameSymlinkToFileItLinksTo - when rename fails, should the test check for exception? Is this part of another jira? Why is creating dangling links allowed? Tests are hard to follow without additional descprition in the test methods what is being tested and what is expected. It took me a while to follow the tests.
          Hide
          Suresh Srinivas added a comment -

          Ignore my comment 3, since symlinks should allow creation of dangling links.

          Show
          Suresh Srinivas added a comment - Ignore my comment 3, since symlinks should allow creation of dangling links.
          Hide
          Eli Collins added a comment -

          The updated table is more clear, thanks. Modified tests like testRenameSymlinkToFileItLinksTo check the particular exception thrown (like the tests that cover renaming a file, directory, etc to itself). Added additional comments to the tests and fixed the typo. Patch attached.

          Show
          Eli Collins added a comment - The updated table is more clear, thanks. Modified tests like testRenameSymlinkToFileItLinksTo check the particular exception thrown (like the tests that cover renaming a file, directory, etc to itself). Added additional comments to the tests and fixed the typo. Patch attached.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Suresh Srinivas added a comment -

          resubmitting the patch to trigger hudson tests.

          Show
          Suresh Srinivas added a comment - resubmitting the patch to trigger hudson tests.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12443160/hadoop-6563-5.patch
          against trunk revision 939510.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/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/12443160/hadoop-6563-5.patch against trunk revision 939510. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/491/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          The TestFilterFileSystem failure is unrelated to this change, fails on trunk after HADOOP-6709. I'll track that down.

          Show
          Eli Collins added a comment - The TestFilterFileSystem failure is unrelated to this change, fails on trunk after HADOOP-6709 . I'll track that down.
          Hide
          Suresh Srinivas added a comment -

          I committed the patch. Thank you Eli.

          Show
          Suresh Srinivas added a comment - I committed the patch. Thank you Eli.

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development