Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.20.1, 0.21.0, 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: contrib/streaming
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Many of the streaming tests (including TestMultipleArchiveFiles) catch exceptions and print their stack trace rather than failing the job. This means that tests do not fail even when the job fails.

      1. TestStreamingExitStatus.patch
        3 kB
        Devaraj Das
      2. mapreduce-1155.txt
        38 kB
        Todd Lipcon
      3. mapreduce-1155.txt
        61 kB
        Todd Lipcon
      4. mapreduce-1155.patch
        60 kB
        Todd Lipcon
      5. mapreduce-1155.patch
        64 kB
        Todd Lipcon

        Activity

        Hide
        Todd Lipcon added a comment -

        This patch cleans up a bunch of the streaming test code. Primary changes:

        • Got rid of a bunch of instances where exceptions were just printed and didn't fail the tests.
        • Fixed StreamJob so that IllegalArgumentExceptions are only swallowed during the argument parsing, and not when they come from inside MR. (this is the only change to non-test code)
        • Fixed a bunch of instances where a failTrace() method had been copy pasted and used in exception catch blocks. This is unnecessary in tests - simply letting it bubble up to the test harness is more idiomatic.
        • Fixed a bug where TestMultipleArchivefiles was actually failing the job

        These tests could do with a whole lot more cleanup work, but this is a good start and at least makes them fail properly when there are problems.

        Show
        Todd Lipcon added a comment - This patch cleans up a bunch of the streaming test code. Primary changes: Got rid of a bunch of instances where exceptions were just printed and didn't fail the tests. Fixed StreamJob so that IllegalArgumentExceptions are only swallowed during the argument parsing, and not when they come from inside MR. (this is the only change to non-test code) Fixed a bunch of instances where a failTrace() method had been copy pasted and used in exception catch blocks. This is unnecessary in tests - simply letting it bubble up to the test harness is more idiomatic. Fixed a bug where TestMultipleArchivefiles was actually failing the job These tests could do with a whole lot more cleanup work, but this is a good start and at least makes them fail properly when there are problems.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12426856/mapreduce-1155.txt
        against trunk revision 886353.

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

        +1 tests included. The patch appears to include 42 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/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/12426856/mapreduce-1155.txt against trunk revision 886353. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/287/console This message is automatically generated.
        Hide
        Chris Douglas added a comment -

        This is cleaner than the failTrace idiom in most of the tests. Since you're tightening these up, would you mind updating the tests to use JUnit4 annotations instead of extending TestCase? Other than that, this looks fine

        Show
        Chris Douglas added a comment - This is cleaner than the failTrace idiom in most of the tests. Since you're tightening these up, would you mind updating the tests to use JUnit4 annotations instead of extending TestCase? Other than that, this looks fine
        Hide
        Todd Lipcon added a comment -

        Chris: mind if we do that in a separate JIRA? I opened MAPREDUCE-1268. We may as well fix the broken tests now when there's a patch that applies and passes, and worry about style separately.

        Show
        Todd Lipcon added a comment - Chris: mind if we do that in a separate JIRA? I opened MAPREDUCE-1268 . We may as well fix the broken tests now when there's a patch that applies and passes, and worry about style separately.
        Hide
        Chris Douglas added a comment -

        We may as well fix the broken tests now when there's a patch that applies and passes, and worry about style separately.

        Only some of the tests this is updating are "broken." Most just use an odd and unnecessary idiom. Style is what this is repairing

        Show
        Chris Douglas added a comment - We may as well fix the broken tests now when there's a patch that applies and passes, and worry about style separately. Only some of the tests this is updating are "broken." Most just use an odd and unnecessary idiom. Style is what this is repairing
        Hide
        Todd Lipcon added a comment -

        New patch updates most of the tests to JUnit 4. The one I didn't change was TestStreamingBadRecords since it inherits from org.apache.hadoop.mapred.ClusterMapReduceTestCase which is old-style.

        There's plenty more work that could be done to clean up these tests, for sure, but that's about all I can handle for now

        Show
        Todd Lipcon added a comment - New patch updates most of the tests to JUnit 4. The one I didn't change was TestStreamingBadRecords since it inherits from org.apache.hadoop.mapred.ClusterMapReduceTestCase which is old-style. There's plenty more work that could be done to clean up these tests, for sure, but that's about all I can handle for now
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12427535/mapreduce-1155.txt
        against trunk revision 888761.

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

        +1 tests included. The patch appears to include 87 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 appears to cause Findbugs to fail.

        +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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/testReport/
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/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/12427535/mapreduce-1155.txt against trunk revision 888761. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 87 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 appears to cause Findbugs to fail. +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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/testReport/ Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/180/console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        something went awry with my patch upload. cancelling, will upload a new one tomorrow.

        Show
        Todd Lipcon added a comment - something went awry with my patch upload. cancelling, will upload a new one tomorrow.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12428996/mapreduce-1155.patch
        against trunk revision 893828.

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

        +1 tests included. The patch appears to include 84 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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/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/12428996/mapreduce-1155.patch against trunk revision 893828. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 84 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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/347/console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Turns out the previous test failures were due to test interactions (the individual tests passed, but when run all together they failed). This new version puts each test into its own subdirectory which is cleaned up first so orphaned crc32 files don't cause checksum errors.

        Show
        Todd Lipcon added a comment - Turns out the previous test failures were due to test interactions (the individual tests passed, but when run all together they failed). This new version puts each test into its own subdirectory which is cleaned up first so orphaned crc32 files don't cause checksum errors.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429041/mapreduce-1155.patch
        against trunk revision 894165.

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

        +1 tests included. The patch appears to include 87 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/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/12429041/mapreduce-1155.patch against trunk revision 894165. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 87 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/349/console This message is automatically generated.
        Hide
        Karthik K added a comment -

        Quick look at a test case( TestStreamingExitStatus ) means it is doing the cleanup in the setUp() method to make sure things are ok ( like deleting directories etc.) .

        There is a missing tearDown() method that needs to handle the unwinding of the changes, and setUp() would probably just assert its assumptions before starting afresh.

        Also this patch seems to address more than one issue at hand.

        If this makes the build green - can we place have it committed on the trunk now and revisit piecemeal, since the trunk build has been broken for quite some time.

        Show
        Karthik K added a comment - Quick look at a test case( TestStreamingExitStatus ) means it is doing the cleanup in the setUp() method to make sure things are ok ( like deleting directories etc.) . There is a missing tearDown() method that needs to handle the unwinding of the changes, and setUp() would probably just assert its assumptions before starting afresh. Also this patch seems to address more than one issue at hand. If this makes the build green - can we place have it committed on the trunk now and revisit piecemeal, since the trunk build has been broken for quite some time.
        Hide
        Todd Lipcon added a comment -

        Yes, I elected to do the cleanup in the setup method as a bandaid - I couldn't figure out which tests were leaving junk lying around, so I did the recursive delete at the start. I also sometimes find this technique to be handy since the developer can investigate the state of the test directory after the test has failed. If there's a tearDown method, the state that contained the failure is gone by the time the developer sees that the test failed.

        Also this patch seems to address more than one issue at hand.

        Yep - once I fixed the original issue (swallowing exceptions) I found that some of the tests had been failing before and no one ever knew, so I had to go and fix those as well.

        Show
        Todd Lipcon added a comment - Yes, I elected to do the cleanup in the setup method as a bandaid - I couldn't figure out which tests were leaving junk lying around, so I did the recursive delete at the start. I also sometimes find this technique to be handy since the developer can investigate the state of the test directory after the test has failed. If there's a tearDown method, the state that contained the failure is gone by the time the developer sees that the test failed. Also this patch seems to address more than one issue at hand. Yep - once I fixed the original issue (swallowing exceptions) I found that some of the tests had been failing before and no one ever knew, so I had to go and fix those as well.
        Hide
        Karthik K added a comment -

        Yes, I elected to do the cleanup in the setup method as a bandaid - I couldn't figure out which tests were leaving junk lying around, so I did the recursive delete at the start. I also sometimes find this technique to be handy since the developer can investigate the state of the test directory after the test has failed. If there's a tearDown method, the state that contained the failure is gone by the time the developer sees that the test failed.

        I would vote for a failure of tearDown method by a given test case , so when I see a batch of test failures , after a tearDown failure - I can make a reasonable assumption that there is only 1 base issue that we are concerned about , and the rest may be bogus. Case in point: I was trying to TestStreamingExitStatus / TestStreamingKeyValue test cases. The former did not have a proper cleanup , while the latter failed because of the same. It took me quite a lot of time before concluding that the latter does not have any issue by itself except for incorrect test fixture assumptions.

        But as I said - this is something that can be taken up in a separate bug - but given that this patch seems to make the build green and address the primary issue of swallowing exceptions I would say - go ahead and commit this while we discuss the fixtures in a separate bug altogether.

        Show
        Karthik K added a comment - Yes, I elected to do the cleanup in the setup method as a bandaid - I couldn't figure out which tests were leaving junk lying around, so I did the recursive delete at the start. I also sometimes find this technique to be handy since the developer can investigate the state of the test directory after the test has failed. If there's a tearDown method, the state that contained the failure is gone by the time the developer sees that the test failed. I would vote for a failure of tearDown method by a given test case , so when I see a batch of test failures , after a tearDown failure - I can make a reasonable assumption that there is only 1 base issue that we are concerned about , and the rest may be bogus. Case in point: I was trying to TestStreamingExitStatus / TestStreamingKeyValue test cases. The former did not have a proper cleanup , while the latter failed because of the same. It took me quite a lot of time before concluding that the latter does not have any issue by itself except for incorrect test fixture assumptions. But as I said - this is something that can be taken up in a separate bug - but given that this patch seems to make the build green and address the primary issue of swallowing exceptions I would say - go ahead and commit this while we discuss the fixtures in a separate bug altogether.
        Hide
        Todd Lipcon added a comment -

        go ahead and commit this while we discuss the fixtures in a separate bug altogether.

        Cool. I would wholeheartedly support a complete rewrite of the streaming tests. And a rewrite of streaming itself, but that's a separate story

        Now we just need a committer

        Show
        Todd Lipcon added a comment - go ahead and commit this while we discuss the fixtures in a separate bug altogether. Cool. I would wholeheartedly support a complete rewrite of the streaming tests. And a rewrite of streaming itself, but that's a separate story Now we just need a committer
        Hide
        Karthik K added a comment -

        I am willing to take the bait as I have a definite vested interest in contrib/streaming and contrib/index and I can whip up some stuff after you commit.

        Thanks for the help.

        Show
        Karthik K added a comment - I am willing to take the bait as I have a definite vested interest in contrib/streaming and contrib/index and I can whip up some stuff after you commit. Thanks for the help.
        Hide
        Karthik K added a comment -

        Are we waiting for more code review before this goes into the tree. If this makes the build green- when can we get this committed. thanks.

        Show
        Karthik K added a comment - Are we waiting for more code review before this goes into the tree. If this makes the build green- when can we get this committed. thanks.
        Hide
        Tom White added a comment -

        I've just committed this. Thanks Todd!

        Show
        Tom White added a comment - I've just committed this. Thanks Todd!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #196 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/196/)
        . Streaming tests swallow exceptions. Contributed by Todd Lipcon.

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #196 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/196/ ) . Streaming tests swallow exceptions. Contributed by Todd Lipcon.
        Show
        Tom White added a comment - Hudson's build is now passing: http://hudson.zones.apache.org/hudson/view/Hadoop/job/Hadoop-Mapreduce-trunk/196/
        Hide
        Devaraj Das added a comment -

        Attaching a patch for TestStreamingExitStatus test only (factored out the relevant part from the patch that got committed).

        Show
        Devaraj Das added a comment - Attaching a patch for TestStreamingExitStatus test only (factored out the relevant part from the patch that got committed).

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development