Hadoop Common
  1. Hadoop Common
  2. HADOOP-8801

ExitUtil#terminate should capture the exception stack trace

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      ExitUtil#terminate(status,Throwable) should capture and log the stack trace of the given throwable. This will help debug issues like HDFS-3933.

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Patch attached.

          Show
          Eli Collins added a comment - Patch attached.
          Hide
          Karthik Kambatla added a comment -

          +1

          Show
          Karthik Kambatla 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/12544995/hadoop-8801.txt
          against trunk revision .

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1453//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1453//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/12544995/hadoop-8801.txt against trunk revision . +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 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1453//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1453//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          +1, the patch looks good to me.

          Show
          Aaron T. Myers added a comment - +1, the patch looks good to me.
          Hide
          Eli Collins added a comment -

          Thanks for the reviews guys. I've committed this and merged to branch-2 and branch-2.0.2-alpha.

          Show
          Eli Collins added a comment - Thanks for the reviews guys. I've committed this and merged to branch-2 and branch-2.0.2-alpha.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2729 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2729/)
          HADOOP-8801. ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2729 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2729/ ) HADOOP-8801 . ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2792 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2792/)
          HADOOP-8801. ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2792 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2792/ ) HADOOP-8801 . ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2753 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2753/)
          HADOOP-8801. ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2753 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2753/ ) HADOOP-8801 . ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Hide
          Steve Loughran added a comment -

          looks OK, but the patch should fix something the original got wrong -it should use Throwable.toString() not Throwable.getMessage().

          If you want to see why, look at the source for NullPointerException and work out what message it prints...

          Show
          Steve Loughran added a comment - looks OK, but the patch should fix something the original got wrong -it should use Throwable.toString() not Throwable.getMessage() . If you want to see why, look at the source for NullPointerException and work out what message it prints...
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1165 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1165/)
          HADOOP-8801. ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1165 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1165/ ) HADOOP-8801 . ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1196 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1196/)
          HADOOP-8801. ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1196 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1196/ ) HADOOP-8801 . ExitUtil#terminate should capture the exception stack trace. Contributed by Eli Collins (Revision 1384435) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1384435 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java
          Hide
          Steve Loughran added a comment -

          @Eli, this patch went in within 2 hours of being submitted. This effectively prevented any review from anyone not in PST timezone keeping up to date with their JIRA issues.

          While I celebrate a rapid integration of patches into the tree, I believe this devalues the RTC process, as people like myself can't review, even though as I did belatedly comment the patch should use toString() over getMessage(), because getMessage() has the right to return null.

          1. I think this a bad precedent. It means there's nothing to stop me getting together with someone else in the EU and pushing through a set of changes before anyone notices.
          2. As I said, the patch is inadequate.

          I don't want to revert the patch it's in but I'd like my feedback to be incorporated into a new JIRA, as s/getMessage()/r/toString() enhances the value of the output even more.

          Do you want to do this? Or shall I? And in either case, can we have slightly more than 2h for review?

          Show
          Steve Loughran added a comment - @Eli, this patch went in within 2 hours of being submitted. This effectively prevented any review from anyone not in PST timezone keeping up to date with their JIRA issues. While I celebrate a rapid integration of patches into the tree, I believe this devalues the RTC process, as people like myself can't review, even though as I did belatedly comment the patch should use toString() over getMessage() , because getMessage() has the right to return null. I think this a bad precedent. It means there's nothing to stop me getting together with someone else in the EU and pushing through a set of changes before anyone notices. As I said, the patch is inadequate. I don't want to revert the patch it's in but I'd like my feedback to be incorporated into a new JIRA, as s/getMessage()/r/toString() enhances the value of the output even more. Do you want to do this? Or shall I? And in either case, can we have slightly more than 2h for review?
          Hide
          Aaron T. Myers added a comment -

          Hey Steve, I'm sure Eli will be happy to address your feedback in a follow-up JIRA. That said, for very small and low-risk patches such as this, I don't think that waiting some arbitrary amount of time to allow for someone to comment on it, who might never do so, is productive for anyone. For larger, riskier, or more controversial patches, I'll often say something like "I'll commit this later today/tomorrow unless there are further comments."

          Show
          Aaron T. Myers added a comment - Hey Steve, I'm sure Eli will be happy to address your feedback in a follow-up JIRA. That said, for very small and low-risk patches such as this, I don't think that waiting some arbitrary amount of time to allow for someone to comment on it, who might never do so, is productive for anyone. For larger, riskier, or more controversial patches, I'll often say something like "I'll commit this later today/tomorrow unless there are further comments."
          Hide
          Eli Collins added a comment -

          @Steve,

          I'm happy to address your feedback in a follow up jira.

          On a related note would you be willing to make sure all your changes have been code reviewed? Most of your recent changes (HADOOP-8064, HADOOP-7878, HADOOP-7777, HADOOP-7772, HADOOP-7727, HADOOP-7705, etc) have been committed without any code review or a +1 from another committer, which is our project's policy as I understand it. I'm a little surprised you want more time to have this change code reviewed when you commit your own changes without any code review. Likewise, I don't want to revert these changes but they should not have been committed without review and a +1 from another committer.

          Show
          Eli Collins added a comment - @Steve, I'm happy to address your feedback in a follow up jira. On a related note would you be willing to make sure all your changes have been code reviewed? Most of your recent changes ( HADOOP-8064 , HADOOP-7878 , HADOOP-7777 , HADOOP-7772 , HADOOP-7727 , HADOOP-7705 , etc) have been committed without any code review or a +1 from another committer, which is our project's policy as I understand it. I'm a little surprised you want more time to have this change code reviewed when you commit your own changes without any code review. Likewise, I don't want to revert these changes but they should not have been committed without review and a +1 from another committer.
          Hide
          Eli Collins added a comment -

          Filed HADOOP-8812 to address using toString rather than getMessage.

          Show
          Eli Collins added a comment - Filed HADOOP-8812 to address using toString rather than getMessage.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development