Hadoop Common
  1. Hadoop Common
  2. HADOOP-10571

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

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.4.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      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.

          People

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

            Dates

            • Created:
              Updated:

              Development