Hadoop Common
  1. Hadoop Common
  2. HADOOP-6730

Bug in FileContext#copy and provide base class for FileContext tests

    Details

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

      Description

      Thanks to Eli, He noticed that there is no test for FileContext#Copy operation.

      On further investigation with the help of Sanjay we found that there is bug in FileContext#checkDest.

        FileStatus dstFs = getFileStatus(dst);
          try {
            if (dstFs.isDir()) {
              if (null == srcNa
      

      FileStatus dstFs = getFileStatus(dst); should be in try...catch block.

          try {
             FileStatus dstFs = getFileStatus(dst);
             if (dstFs.isDir()) {
                if (null == srcNa
      
      1. HADOOP-6730.4.patch
        6 kB
        Ravi Phulari
      2. HADOOP-6730.3.patch
        6 kB
        Ravi Phulari
      3. HADOOP-6730.2.patch
        7 kB
        Ravi Phulari
      4. HADOOP-6730.patch
        5 kB
        Ravi Phulari

        Activity

        Hide
        Jakob Homan added a comment -

        I have committed this. Thanks, Ravi. Resolving as fixed.

        Show
        Jakob Homan added a comment - I have committed this. Thanks, Ravi. Resolving as fixed.
        Hide
        Jakob Homan added a comment -

        +1

        Show
        Jakob Homan added a comment - +1
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 9 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-h4.grid.sp2.yahoo.net/492/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/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/12443315/HADOOP-6730.4.patch against trunk revision 939864. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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-h4.grid.sp2.yahoo.net/492/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/492/console This message is automatically generated.
        Hide
        Ravi Phulari added a comment -

        Submitting patch for hudson queue.

        Show
        Ravi Phulari added a comment - Submitting patch for hudson queue.
        Hide
        Ravi Phulari added a comment -

        Updated patch.
        Removed static from fc initialization.
        Removed invalid comments.

        Show
        Ravi Phulari added a comment - Updated patch. Removed static from fc initialization. Removed invalid comments.
        Hide
        Ravi Phulari added a comment -

        Updated patch with changes.
        1. Removed unnecessary tearDown method.
        2. Changed file read write to bytes from String.

        Show
        Ravi Phulari added a comment - Updated patch with changes. 1. Removed unnecessary tearDown method. 2. Changed file read write to bytes from String.
        Hide
        Ravi Phulari added a comment -

        Thanks for reviewing patch Jakob.
        Uploading updated patch.

        Show
        Ravi Phulari added a comment - Thanks for reviewing patch Jakob. Uploading updated patch.
        Hide
        Jakob Homan added a comment -

        Review of the patch:

        • Lines 47-52: These notes aren't necessary since the test writers should know about @Before and @After. Also, the provided methods (setUp() and tearDown()) conflict with the provided names.
        • Lines 55 & 56: These values should be all capitalized and final. Alternatively, it may be good to provide these values via functions so that implementing classes may override as needed.
        • Line 64: catch should be on the same line as closing brace, per our coding style
        • Line 70: fc should not be declared static. It is the responsibility of each implementing class to provide an instance of it. Also, fc should probably be protected
        • Line 91: Please provide assert message for assertion failure
        • Since this is a test that the copy method worked, we should prove it by comparing the contents of the copied file with the original.
        • Lines 135-138: This code:
          @After
          public void tearDown() throws Exception {
            super.tearDown();
          }

          is a no-op and should be removed.

        Nits:

        • Rather than Assert.assertTrue() you can do a static import of org.junit.Assert. {assertTrue|*}

          .

        • Line 57: Remove extra line
        • Line 47: "Since this a junit 4" ->"Since this is a junit 4 test"
        Show
        Jakob Homan added a comment - Review of the patch: Lines 47-52: These notes aren't necessary since the test writers should know about @Before and @After. Also, the provided methods (setUp() and tearDown()) conflict with the provided names. Lines 55 & 56: These values should be all capitalized and final. Alternatively, it may be good to provide these values via functions so that implementing classes may override as needed. Line 64: catch should be on the same line as closing brace, per our coding style Line 70: fc should not be declared static. It is the responsibility of each implementing class to provide an instance of it. Also, fc should probably be protected Line 91: Please provide assert message for assertion failure Since this is a test that the copy method worked, we should prove it by comparing the contents of the copied file with the original. Lines 135-138: This code: @After public void tearDown() throws Exception { super .tearDown(); } is a no-op and should be removed. Nits: Rather than Assert.assertTrue() you can do a static import of org.junit.Assert. {assertTrue|*} . Line 57: Remove extra line Line 47: "Since this a junit 4" ->"Since this is a junit 4 test"
        Hide
        Eli Collins added a comment -

        +1 Looks good to me.

        Show
        Eli Collins added a comment - +1 Looks good to me.
        Hide
        Ravi Phulari added a comment -

        FileContext.Util.Copy() operation was failing with file not found exception if destination file does not exits , this was happening due to the fact that getFileStatus was out of try block.

        The patch just has util classes - perhaps you forgot to add the test class in your diff?

        I have included test runner with util base. Please check that there are two classes added with this patch - TestFcLocalFsUtil.java and FileContextUtilBase.java

        Show
        Ravi Phulari added a comment - FileContext.Util.Copy() operation was failing with file not found exception if destination file does not exits , this was happening due to the fact that getFileStatus was out of try block. The patch just has util classes - perhaps you forgot to add the test class in your diff? I have included test runner with util base. Please check that there are two classes added with this patch - TestFcLocalFsUtil.java and FileContextUtilBase.java
        Hide
        Eli Collins added a comment -

        Hey Ravi, per the previous comment I left the getFileStatus outside the try block so the FileNotFoundException is now swallowed silently and is thrown up to copy (whose API wants FileNotFoundThrown). This deserves a comment.

        The patch just has util classes - perhaps you forgot to add the test class in your diff?

        Show
        Eli Collins added a comment - Hey Ravi, per the previous comment I left the getFileStatus outside the try block so the FileNotFoundException is now swallowed silently and is thrown up to copy (whose API wants FileNotFoundThrown). This deserves a comment. The patch just has util classes - perhaps you forgot to add the test class in your diff?
        Hide
        Ravi Phulari added a comment -

        Attaching patch for review.

        Show
        Ravi Phulari added a comment - Attaching patch for review.
        Hide
        Eli Collins added a comment -

        The call to getFileStatus is outside the try block to make sure FileNotFoundException is thrown per the interface no?

        Show
        Eli Collins added a comment - The call to getFileStatus is outside the try block to make sure FileNotFoundException is thrown per the interface no?
        Hide
        Ravi Phulari added a comment -

        Adding information about bug in FileContext#checkDest.

        Will upload patch soon with bug fix and Tests for copy.

        Show
        Ravi Phulari added a comment - Adding information about bug in FileContext#checkDest. Will upload patch soon with bug fix and Tests for copy.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development