Hadoop Common
  1. Hadoop Common
  2. HADOOP-10571

Use Log.*(Object, Throwable) overload to log exceptions

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.4.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      When logging an exception, we often convert the exception to string or call .getMessage. Instead we can use the log method overloads which take Throwable as a parameter.

        Activity

        Hide
        Suresh Srinivas added a comment -

        There are some place where we specifically avoid printing stack trace. When making this change, we need to be careful to keep the exception printing terse where necessary.

        Show
        Suresh Srinivas added a comment - There are some place where we specifically avoid printing stack trace. When making this change, we need to be careful to keep the exception printing terse where necessary.
        Hide
        Arpit Agarwal added a comment -

        Initial patch to fix logging statements in hadoop-common, hadoop-hdfs and hadoop-nfs.

        In some locations it seemed to be a conscious choice to avoid logging the full stack trace, so I left them unmodified.

        Show
        Arpit Agarwal added a comment - Initial patch to fix logging statements in hadoop-common, hadoop-hdfs and hadoop-nfs. In some locations it seemed to be a conscious choice to avoid logging the full stack trace, so I left them unmodified.
        Hide
        Suresh Srinivas added a comment -

        This might be an opportunity to add a comment to that says the exception is terse in Log message by design to avoid someone changing it to verbose stack trace.

        Show
        Suresh Srinivas added a comment - This might be an opportunity to add a comment to that says the exception is terse in Log message by design to avoid someone changing it to verbose stack trace.
        Hide
        Arpit Agarwal added a comment -

        Thanks Suresh, I used my judgement where it wasn't obvious. Please let me know of any unnecessary edits if you get a chance to review.

        I used regular expressions to look for candidates so I could have missed some.

        Show
        Arpit Agarwal added a comment - Thanks Suresh, I used my judgement where it wasn't obvious. Please let me know of any unnecessary edits if you get a chance to review. I used regular expressions to look for candidates so I could have missed some.
        Hide
        Steve Loughran added a comment -

        Arpit, we had a discussion on common-dev about actually switching to SLF4J -its on the classpath. If we are going to rework exceptions it lets us

        1. varags
        2. skip the is*Enabled checks -at least on info err and warn
        3. rely on dynamic string expansion with handling for nulls and first element of arrays
        4. let you include the text of an exception in the short message, as well as its stack (it looks for a throwable as the last arg and prints the stack trace.
        5. still compatible at source level with commons-logging

        together this would let you do

        log.info("exception connecting to {} with timeout :{},host, time, e"). 
        
        Show
        Steve Loughran added a comment - Arpit, we had a discussion on common-dev about actually switching to SLF4J -its on the classpath. If we are going to rework exceptions it lets us varags skip the is*Enabled checks -at least on info err and warn rely on dynamic string expansion with handling for nulls and first element of arrays let you include the text of an exception in the short message, as well as its stack (it looks for a throwable as the last arg and prints the stack trace. still compatible at source level with commons-logging together this would let you do log.info( "exception connecting to {} with timeout :{},host, time, e" ).
        Hide
        Arpit Agarwal added a comment -

        Steve, I just read through the thread and checked out the SLF4J API.

        It looks like we'd have to migrate to SLF4J on a per-file basis. Could we just rework the exceptions for now and do the migration separately?

        Show
        Arpit Agarwal added a comment - Steve, I just read through the thread and checked out the SLF4J API. It looks like we'd have to migrate to SLF4J on a per-file basis. Could we just rework the exceptions for now and do the migration separately?
        Hide
        Suresh Srinivas added a comment -

        I agree with Arpit Agarwal. Lets decouple the current set of improvements from SLF4J.

        Show
        Suresh Srinivas added a comment - I agree with Arpit Agarwal . Lets decouple the current set of improvements from SLF4J.
        Hide
        Steve Loughran added a comment -

        Fine, but as the patches are adding new string contats it would seem to make sense to do it now on the files being changed

        Show
        Steve Loughran added a comment - Fine, but as the patches are adding new string contats it would seem to make sense to do it now on the files being changed
        Hide
        Arpit Agarwal added a comment -

        Hi Suresh Srinivas / Steve Loughran, if you don't object could you please +1 the change? Steve, I'll volunteer to move this module to SLF4J separately.

        Show
        Arpit Agarwal added a comment - Hi Suresh Srinivas / Steve Loughran , if you don't object could you please +1 the change? Steve, I'll volunteer to move this module to SLF4J separately.
        Hide
        Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        -1 patch 0m 0s The patch command could not apply the patch during dryrun.



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12643122/HADOOP-10571.01.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / f1a152c
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6330/console

        This message was automatically generated.

        Show
        Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 patch 0m 0s The patch command could not apply the patch during dryrun. Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12643122/HADOOP-10571.01.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f1a152c Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6330/console This message was automatically generated.
        Hide
        Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        -1 patch 0m 0s The patch command could not apply the patch during dryrun.



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12643122/HADOOP-10571.01.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / f1a152c
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6340/console

        This message was automatically generated.

        Show
        Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 patch 0m 0s The patch command could not apply the patch during dryrun. Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12643122/HADOOP-10571.01.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f1a152c Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6340/console This message was automatically generated.
        Hide
        Steve Loughran added a comment -
        1. I've found the combo of log(text+exception.toString, exception) handy for those situations where the log gets lost
        2. This could be an opportunity for SLF4J-ing those classes which are being patched. That's initially just changing the import & log assignment, – but for changed lines lets us go from log.info("text" + a + " " + ex, ex) to log.info("text {}, {}", a, ex, ex and have the expansion only done when needed
        Show
        Steve Loughran added a comment - I've found the combo of log(text+exception.toString, exception) handy for those situations where the log gets lost This could be an opportunity for SLF4J-ing those classes which are being patched. That's initially just changing the import & log assignment, – but for changed lines lets us go from log.info("text" + a + " " + ex, ex) to log.info("text {}, {}", a, ex, ex and have the expansion only done when needed

          People

          • Assignee:
            Arpit Agarwal
            Reporter:
            Arpit Agarwal
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development