Kafka
  1. Kafka
  2. KAFKA-193

use by name parameter helper for logging and trait to include lazy logging and refactor code to use the new LogHelper

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:
      None

      Description

      1) New tait to include logging and helper methods so if (log.isDebugEnabled()) is not required because it is in the helper and log paramaters are passed by name so not executed to tidy up the code

      2) refactor all occurrence of logging to use the log helper

      3/4 (possibly to be handled in to tickets) the "lint" affect from this for changes patched but not on trunk and new patches moving forward until this is baked in

      1. kafka-193-consistent-level-throwable.fix.v2.patch
        156 kB
        Jay Kreps
      2. kafka-193-consistent-level-throwable.fix.patch
        156 kB
        Joe Stein
      3. kafka-193-consistent-level-throwable.patch
        156 kB
        Joe Stein
      4. kafka-193.patch
        155 kB
        Joe Stein

        Activity

        Hide
        Joe Stein added a comment -

        new trait in utils/Logging.scala

        scoured trunk and re-factored to use trait

        handling the incremental updates for patches that go to trunk before this patch can maybe just be more patches on this ticket. after this patch goes to trunk it would be great that any new patches start to use the trait.

        Show
        Joe Stein added a comment - new trait in utils/Logging.scala scoured trunk and re-factored to use trait handling the incremental updates for patches that go to trunk before this patch can maybe just be more patches on this ticket. after this patch goes to trunk it would be great that any new patches start to use the trait.
        Hide
        Jay Kreps added a comment -

        This looks good to me.

        One thing I notice is that the type of the argument is, for example,
        debug(m: =>String)
        but the type in log4j is
        void debug(Object m)

        Is this intentional? There are two impacts from this:

        Good: people will no longer do
        error(e)
        The problem with this is that I think it uses e.toString which doesn't print the stack trace but just prints the message.

        Bad: I can no longer print a non-string without calling toString, e.g.
        debug(kafkaRequestObj)

        Show
        Jay Kreps added a comment - This looks good to me. One thing I notice is that the type of the argument is, for example, debug(m: =>String) but the type in log4j is void debug(Object m) Is this intentional? There are two impacts from this: Good: people will no longer do error(e) The problem with this is that I think it uses e.toString which doesn't print the stack trace but just prints the message. Bad : I can no longer print a non-string without calling toString, e.g. debug(kafkaRequestObj)
        Hide
        Joe Stein added a comment -

        Well, error and fatal have overloaded for throwable since existing code used this... error(e) can still be done the old way (I was coding to keep consistent but we can improve some too)

        Yes, toString() on the throwable object just is a short description, the message, yup. Bad (my opinion)

        debug(m: => String) did not get debug(m: => Throwable) because no one was doing that yet in the code.

        as far as good, my preference/opinion is if you are going to log something to make it useful and informational. stack traces are best

        if we accept throwable for every level we could always take that object and do getStackTrace().toString() http://download.oracle.com/javase/6/docs/api/java/lang/Throwable.html#getStackTrace() and then log that result underneath them

        e.g.

        def debug(e: => Throwable): Any =

        { logger.debug(e.getStackTrace().toString()) }

        so folks can do error(e) and we can still get what we want/need in the logs

        so how about I change all of the levels in LogHelper to accept throwable as a paramater and to use this stacktrace extraction and to string that for logging?

        anything else?

        Show
        Joe Stein added a comment - Well, error and fatal have overloaded for throwable since existing code used this... error(e) can still be done the old way (I was coding to keep consistent but we can improve some too) Yes, toString() on the throwable object just is a short description, the message, yup. Bad (my opinion) debug(m: => String) did not get debug(m: => Throwable) because no one was doing that yet in the code. as far as good, my preference/opinion is if you are going to log something to make it useful and informational. stack traces are best if we accept throwable for every level we could always take that object and do getStackTrace().toString() http://download.oracle.com/javase/6/docs/api/java/lang/Throwable.html#getStackTrace( ) and then log that result underneath them e.g. def debug(e: => Throwable): Any = { logger.debug(e.getStackTrace().toString()) } so folks can do error(e) and we can still get what we want/need in the logs so how about I change all of the levels in LogHelper to accept throwable as a paramater and to use this stacktrace extraction and to string that for logging? anything else?
        Hide
        Jay Kreps added a comment -

        Yeah so my recommendations.
        1. Let's have the same variants for each level for consistency. Sounds like that is info(m:=>String), info(m:=>String, t: Throwable) and info(t: Throwable).
        2. I am cool with either String or Object as the message type (e.g. info(m:=>String) or info(m:=>Object). I don't see a great benefit of the Object type in log4j.
        3. Let's definitely make the form that takes only a throwable print the stack trace. I think t.getStackTrace() won't quite do it because the return value is just an array. We can probably have that do info("", t), which I think would do what we want or we have a utility function which I think formats the stack trace.

        Show
        Jay Kreps added a comment - Yeah so my recommendations. 1. Let's have the same variants for each level for consistency. Sounds like that is info(m:=>String), info(m:=>String, t: Throwable) and info(t: Throwable). 2. I am cool with either String or Object as the message type (e.g. info(m:=>String) or info(m:=>Object). I don't see a great benefit of the Object type in log4j. 3. Let's definitely make the form that takes only a throwable print the stack trace. I think t.getStackTrace() won't quite do it because the return value is just an array. We can probably have that do info("", t), which I think would do what we want or we have a utility function which I think formats the stack trace.
        Hide
        Joe Stein added a comment -

        ok, I redid the LogHelper to have

        1) consistent functions for all levels
        2) format for the throwable object (using toString() on the StackTraceElement http://download.oracle.com/javase/6/docs/api/java/lang/StackTraceElement.html#toString())
        3) formated both when a string and throwable are passed at the same time.
        4) helper functions for 2 and 3

        Show
        Joe Stein added a comment - ok, I redid the LogHelper to have 1) consistent functions for all levels 2) format for the throwable object (using toString() on the StackTraceElement http://download.oracle.com/javase/6/docs/api/java/lang/StackTraceElement.html#toString( )) 3) formated both when a string and throwable are passed at the same time. 4) helper functions for 2 and 3
        Hide
        Jay Kreps added a comment -

        Hmm, have you tried this out? Not sure if that works. You are doing
        def throwableString(e: => Throwable) = e.getStackTrace.toString

        But getStackTrace gives a java array primitive, so that doesn't print a stack trace:

        jkreps-mn:~ jkreps$ scala
        scala> val e = new RuntimeException("Hello")
        e: java.lang.RuntimeException = java.lang.RuntimeException: Hello

        scala> e.getStackTrace.toString
        res8: java.lang.String = [Ljava.lang.StackTraceElement;@4439bc82

        What if instead we stick with log4j and do the following variants (for info/debug/trace/error/fatal):
        def info(m: =>String) = if(logger.isInfoEnabled) logger.info(m)
        def info(m: =>String, e: Throwable) = if(logger.isInfoEnabled) logger.info(m, e)
        def info(e: Throwable) = if(logger.isInfoEnabled) logger.info("", e)

        I think this does what we want with respect to exceptions.

        Show
        Jay Kreps added a comment - Hmm, have you tried this out? Not sure if that works. You are doing def throwableString(e: => Throwable) = e.getStackTrace.toString But getStackTrace gives a java array primitive, so that doesn't print a stack trace: jkreps-mn:~ jkreps$ scala scala> val e = new RuntimeException("Hello") e: java.lang.RuntimeException = java.lang.RuntimeException: Hello scala> e.getStackTrace.toString res8: java.lang.String = [Ljava.lang.StackTraceElement;@4439bc82 What if instead we stick with log4j and do the following variants (for info/debug/trace/error/fatal): def info(m: =>String) = if(logger.isInfoEnabled) logger.info(m) def info(m: =>String, e: Throwable) = if(logger.isInfoEnabled) logger.info(m, e) def info(e: Throwable) = if(logger.isInfoEnabled) logger.info("", e) I think this does what we want with respect to exceptions.
        Hide
        Joe Stein added a comment - - edited

        I tested your approach and that works great just like we want.

        Attached patch "kafka-193-consistent-level-throwable.fix.patch" to coincide with that.

        Show
        Joe Stein added a comment - - edited I tested your approach and that works great just like we want. Attached patch "kafka-193-consistent-level-throwable.fix.patch" to coincide with that.
        Hide
        Jay Kreps added a comment -

        Cool, here is the same patch updated against trunk to fix a few new breakages due to new checkins. I also made two minor changes:
        1. Changed LogHelper trait to Logging and made the file name match the trait name. Motivation here is just to make the name trait-like...
        2. Made Utils.registerMbean not throw an exception. Utils.swallow was depending on log4j and this lead to a lot of unnecessary use of log4j directly. I think it is better for jmx registration to not be a fatal exception (JMX shouldn't kill the server or client, just log an error).

        If no objections to these I am going to apply the updated patch.

        Show
        Jay Kreps added a comment - Cool, here is the same patch updated against trunk to fix a few new breakages due to new checkins. I also made two minor changes: 1. Changed LogHelper trait to Logging and made the file name match the trait name. Motivation here is just to make the name trait-like... 2. Made Utils.registerMbean not throw an exception. Utils.swallow was depending on log4j and this lead to a lot of unnecessary use of log4j directly. I think it is better for jmx registration to not be a fatal exception (JMX shouldn't kill the server or client, just log an error). If no objections to these I am going to apply the updated patch.
        Hide
        Jun Rao added a comment -

        +1 on the latest patch.

        Show
        Jun Rao added a comment - +1 on the latest patch.
        Hide
        Jay Kreps added a comment -

        Epic patch committed.

        Show
        Jay Kreps added a comment - Epic patch committed.
        Hide
        Jun Rao added a comment -

        We are seeing a weird issue related to this patch. The following java class won't compile.

        public class MyTest extends kafka.consumer.storage.sql.OracleOffsetStorage
        {
        public MyTest()

        { super(null); }

        }

        The compilation error is:

        fatal(scala.Function0) in kafka.consumer.storage.sql.OracleOffsetStorage cannot implement fatal(scala.Function0<java.lang.String>) in kafka.utils.Logging; attempting to use incompatible return type
        found : java.lang.Object
        required: void

        A similar scala class like the following compiles fine.

        class MyTest() extends kafka.consumer.storage.sql.OracleOffsetStorage(null) {
        }

        Anyone knows the issue?

        Show
        Jun Rao added a comment - We are seeing a weird issue related to this patch. The following java class won't compile. public class MyTest extends kafka.consumer.storage.sql.OracleOffsetStorage { public MyTest() { super(null); } } The compilation error is: fatal(scala.Function0) in kafka.consumer.storage.sql.OracleOffsetStorage cannot implement fatal(scala.Function0<java.lang.String>) in kafka.utils.Logging; attempting to use incompatible return type found : java.lang.Object required: void A similar scala class like the following compiles fine. class MyTest() extends kafka.consumer.storage.sql.OracleOffsetStorage(null) { } Anyone knows the issue?
        Hide
        Joe Stein added a comment -

        The issue looks like it is with Scala 2.8.0

        I build and use a Kafka jar against 2.9.1 and with that Kafka jar your MyTest.java compiles fine

        I also just tried 2.8.2 and that also worked, can you bump your scala version build a jar and use that?

        Show
        Joe Stein added a comment - The issue looks like it is with Scala 2.8.0 I build and use a Kafka jar against 2.9.1 and with that Kafka jar your MyTest.java compiles fine I also just tried 2.8.2 and that also worked, can you bump your scala version build a jar and use that?

          People

          • Assignee:
            Jay Kreps
            Reporter:
            Joe Stein
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development