Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-826

harchive doesn't use ToolRunner / harchive returns 0 even if the job fails with exception

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.2
    • Component/s: harchive
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Use ToolRunner for archives job and return non zero error code on failure.
    1. mapreduce-826-1.patch
      0.8 kB
      Koji Noguchi
    2. mapreduce-826-2.patch
      0.8 kB
      Koji Noguchi

      Activity

      Hide
      Mahadev konar added a comment -

      I just committed this to the 0.20 branch...

      Show
      Mahadev konar added a comment - I just committed this to the 0.20 branch...
      Hide
      Mahadev konar added a comment -

      +1 the tests passed .... except for TestHdfsProxy with ParseCliExceptions (unrelated to the patch)

      Show
      Mahadev konar added a comment - +1 the tests passed .... except for TestHdfsProxy with ParseCliExceptions (unrelated to the patch)
      Hide
      Mahadev konar added a comment -

      +1 ill commit this to 0.20 branch.. after running the tests...

      Show
      Mahadev konar added a comment - +1 ill commit this to 0.20 branch.. after running the tests...
      Hide
      Koji Noguchi added a comment -

      Can we have this backported to 0.20 branch as well?
      Same patch applies.

      1) Probably very rarely, but there's a chance that archive command gets an Exception (besides IOException) and still returns 0 (success).
      Without this patch, I'm not comfortable asking users to start trying archive (and deleting the original files).

      2) Without ToolRunner, users need to explicitly edit a config to run on different mapreduce queues.

      Show
      Koji Noguchi added a comment - Can we have this backported to 0.20 branch as well? Same patch applies. 1) Probably very rarely, but there's a chance that archive command gets an Exception (besides IOException) and still returns 0 (success). Without this patch, I'm not comfortable asking users to start trying archive (and deleting the original files). 2) Without ToolRunner, users need to explicitly edit a config to run on different mapreduce queues.
      Hide
      Hudson added a comment -

      Integrated in Hadoop-Mapreduce-trunk #83 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/83/)
      . harchive doesn't use ToolRunner / harchive returns 0 even if the job fails with exception (koji Noguchi via mahadev)

      Show
      Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #83 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/83/ ) . harchive doesn't use ToolRunner / harchive returns 0 even if the job fails with exception (koji Noguchi via mahadev)
      Hide
      Hudson added a comment -

      Integrated in Hadoop-Mapreduce-trunk-Commit #36 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/36/)
      . harchive doesn't use ToolRunner / harchive returns 0 even if the job fails with exception (koji Noguchi via mahadev)

      Show
      Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #36 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/36/ ) . harchive doesn't use ToolRunner / harchive returns 0 even if the job fails with exception (koji Noguchi via mahadev)
      Hide
      Mahadev konar added a comment -

      I just committed this. thanks koji

      Show
      Mahadev konar added a comment - I just committed this. thanks koji
      Hide
      Mahadev konar added a comment -

      its a very minor change, It should be ok without a test... Ill go ahead and commit it.

      Show
      Mahadev konar added a comment - its a very minor change, It should be ok without a test... Ill go ahead and commit it.
      Hide
      Hadoop QA added a comment -

      -1 overall. Here are the results of testing the latest attachment
      http://issues.apache.org/jira/secure/attachment/12419080/mapreduce-826-2.patch
      against trunk revision 813140.

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

      -1 tests included. The patch doesn't appear to include any new or modified tests.
      Please justify why no new tests are needed for this patch.
      Also please list what manual steps were performed to verify this patch.

      +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-h3.grid.sp2.yahoo.net/22/testReport/
      Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/artifact/trunk/build/test/checkstyle-errors.html
      Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/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/12419080/mapreduce-826-2.patch against trunk revision 813140. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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-h3.grid.sp2.yahoo.net/22/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/22/console This message is automatically generated.
      Hide
      Mahadev konar added a comment -

      +1 looks good...

      Show
      Mahadev konar added a comment - +1 looks good...
      Hide
      Koji Noguchi added a comment -

      Thanks Mahadev. Made the change.

      Since this is a patch around main, didn't find a straight forward way to do a unit test.

      One manual test. Before the patch.
      $ hadoop archive -archiveName myhar.har -p /tmp/somenonexistdir somedir /user/knoguchi
      null
      $ echo $?
      0

      After the patch,

      $ hadoop archive -archiveName myhar.har -p /tmp/somenonexistdir somedir /user/knoguchi
      Exception in archives
      null
      lieliftbean-lm:trunk knoguchi$ echo $?
      1

      I guess we should also fix the NPE when src doesn't exist. I'm leaving it for now since this was a good manual test case.

      Show
      Koji Noguchi added a comment - Thanks Mahadev. Made the change. Since this is a patch around main, didn't find a straight forward way to do a unit test. One manual test. Before the patch. $ hadoop archive -archiveName myhar.har -p /tmp/somenonexistdir somedir /user/knoguchi null $ echo $? 0 After the patch, $ hadoop archive -archiveName myhar.har -p /tmp/somenonexistdir somedir /user/knoguchi Exception in archives null lieliftbean-lm:trunk knoguchi$ echo $? 1 I guess we should also fix the NPE when src doesn't exist. I'm leaving it for now since this was a good manual test case.
      Hide
      Mahadev konar added a comment -

      can we wrap the exception around try and catch and not let main() throw the exception at the console?

      you can do this:

      int ret = 0;
      
      try{
        ret = ToolRunner.run(harchives, args);
      } catch(Exception e) {
        LOG.debug("Exception in archives  ", e);
        System.err.println(e.getLocalizedMessage());
        System.exit(1);
      }
      System.exit(ret);
      
      Show
      Mahadev konar added a comment - can we wrap the exception around try and catch and not let main() throw the exception at the console? you can do this: int ret = 0; try { ret = ToolRunner.run(harchives, args); } catch (Exception e) { LOG.debug( "Exception in archives " , e); System .err.println(e.getLocalizedMessage()); System .exit(1); } System .exit(ret);
      Hide
      Hadoop QA added a comment -

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

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

      -1 tests included. The patch doesn't appear to include any new or modified tests.
      Please justify why no new tests are needed for this patch.
      Also please list what manual steps were performed to verify this patch.

      +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/50/testReport/
      Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/artifact/trunk/build/test/checkstyle-errors.html
      Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/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/12415555/mapreduce-826-1.patch against trunk revision 812546. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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/50/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/50/console This message is automatically generated.
      Hide
      Koji Noguchi added a comment -

      1) Calls ToolRunner.run
      2) Took out catch(Excepttion e) and let main fail with stack dump. At least return value would be non-zero.

      Show
      Koji Noguchi added a comment - 1) Calls ToolRunner.run 2) Took out catch(Excepttion e) and let main fail with stack dump. At least return value would be non-zero.

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development