kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dongjin Lee <dong...@apache.org>
Subject Re: [DISCUSS] KIP-653: Upgrade log4j to log4j2
Date Mon, 28 Sep 2020 16:07:57 GMT
> 3. ... For the same reason I call that a bug, I think the description in
the KIP is
incorrect.

Agree, the description in the KIP is written before you open a PR (
https://github.com/apache/kafka/pull/9266) - As you remember, I am
participating the review. I think it is a bug and should be fixed. (And it
seems like it will be.)

> In any case I think some careful testing to ensure compatibility would be
very beneficial.

Yes, I am now adding some additional verifications to make sure. It is
almost done, and I will update the KIP as soon as I complete them.

Don't hesitate to give me additional comments if it is necessary.

Best,
Dongjin

On Mon, Sep 28, 2020 at 8:03 PM Tom Bentley <tbentley@redhat.com> wrote:

> Hi Dongjin,
>
> Sorry for the late reply.
>
> 1. I think translating the name "root" to "" would work fine.
>
> 2. The second bullet in the Connect section of the KIP seems to need some
> translation between null and OFF, similar to the name translation.
>
> 3. The third bullet seems to be about logger inheritance. As you know, I
> have an open PR (https://github.com/apache/kafka/pull/9266) to fix a bug
> where the broker/connect reports the root logger's level rather than
> respecting the actual (inherited) level of a logger in the hierarchy. For
> the same reason I call that a bug, I think the description in the KIP is
> incorrect. The behaviour change described would seem to be incompatible
> whether the PR was merged or not.
>
> I'm not an expert in log4j, but my understanding is as follows:
>
> * In original log4j, a logger and its configuration were both represented
> by a Logger instance. A Logger could be instantiated and configured
> according to the config file (or programmatically). If it was created by a
> call to the LogManager (e.g. in order to log something) its configuration
> would be inherited. This meant there was only one place to look for a
> loggers level: The Logger itself. This meant that getting or setting a
> logger's level was easy.
>
> * In log4j2 a LoggerConfig (the thing created by the config file) is a
> separate thing from the Logger (the thing on which you call warn(), debug()
> etc) itself and I think this makes it harder to provide compatibility with
> the log4j v1 behaviour for things like getting a logger's level, because
> AFAIK log4j2 doesn't provide a convenient API for doing so. Instead when
> finding a logger's level you have to look for both a LoggerConfig and a
> Logger, because the level could be set in either. This is all based on what
> I learned when I was looking at the log4j2 switch (before I knew you were
> already looking at it, if you recall). I have some code from then [1] which
> may be of use though it's in a bit of a rough state. In any case I think
> some careful testing to ensure compatibility would be very beneficial.
>
> Kind regards,
>
> Tom
>
> [1]:
>
> https://github.com/tombentley/kafka/blob/KAFKA-1368-log4j2/core/src/main/scala/kafka/utils/Log4j2Controller.scala
>
>
>
>
>
> On Wed, Sep 23, 2020 at 1:50 PM Dongjin Lee <dongjin@apache.org> wrote:
>
> > Hi Tom,
> >
> > Thanks for the detailed analysis. Recently, I was also thinking about API
> > compatibility. I initially thought that the difference between the root
> > logger name would break the compatibility (as the KIP states), it seems
> > like I found a workaround:
> >
> > 1. When the user requests arrive, regard the logger name 'root' as an
> empty
> > string. (i.e., translate the request into the log4j2 equivalent.)
> > 2. When generating the response, change the logger name '' into 'root'.
> > (i.e., translate the response into the log4j equivalent.)
> > 3. Remove (or make reverse) the workaround above when we make log4j2
> > default.
> >
> > In short, it seems like we can handle the API incompatibility introduced
> by
> > the root logger name change by adding a facade.
> >
> > How do you think?
> >
> > Thanks,
> > Dongjin
> >
> > On Wed, Sep 23, 2020 at 7:36 PM Tom Bentley <tbentley@redhat.com> wrote:
> >
> > > Hi Dongjin,
> > >
> > > I'd like to see this feature, but if I understand correctly, the KIP in
> > its
> > > current form breaks a couple of Kafka APIs. For Kafka Connect it says
> > "From
> > > log4j2, the name of the root logger becomes empty string from 'root'.
> It
> > > impacts Kafka connect's dynamic logging control feature. (And should be
> > > documented.)". This seems to imply that any tooling that a user might
> > have
> > > written about logging in Kafka Connect will break because the client
> and
> > > server don't have a shared understanding of how to identify the root
> > > logger. The same would be true for the DescribeConfigs, AlterConfigs
> and
> > > IncrementalAlterConfigs protocols using the BROKER_LOGGER resource
> type,
> > I
> > > think.
> > >
> > > Maybe that's OK if the behaviour was changing in a new major release of
> > > Kafka (e.g. 3.0), but I don't think it's allowed in Kafka 2.7 given the
> > > project's compatibility requirements & semantic versioning.
> > >
> > > If these API compatibility issues are easily fixed I think it would be
> > > great to have this in 2.7, but if not it might be easier to target this
> > for
> > > Kafka 3.0. That would also allow you to change the logging config
> format
> > as
> > > suggested by Ismael.
> > >
> > > Many thanks,
> > >
> > > Tom
> > >
> > > On Tue, Sep 22, 2020 at 5:15 PM Dongjin Lee <dongjin@apache.org>
> wrote:
> > >
> > > > Hi devs,
> > > >
> > > > I updated the KIP with the migration plan I discussed with Ismael.
> > > >
> > > > I think 2.7.0 is the perfect time for starting migration into log4j2.
> > If
> > > we
> > > > miss this opportunity, the migration would be much harder. So please
> > > have a
> > > > look at this proposal.
> > > >
> > > > I also opened a voting thread for this.
> > > >
> > > > Thanks,
> > > > Dongjin
> > > >
> > > > On Thu, Sep 17, 2020 at 2:29 AM Dongjin Lee <dongjin@apache.org>
> > wrote:
> > > >
> > > > > Hi Ismael,
> > > > >
> > > > > > Have we considered switching to the log4j2 logging config format
> by
> > > > > default and providing a mechanism to use the old format?
> > > > >
> > > > > As of present, the proposal leaves the default config format
> > switching
> > > to
> > > > > sometime in the future. However, I think it is not a difficult task
> > and
> > > > is
> > > > > up to the community's decision. The draft implementation already
> > > includes
> > > > > log4j2 counterparts for all existing 1.x format (template) configs.
> > > > > Although it still uses traditional log4j format as a default for
> > > backward
> > > > > compatibility, the users who prefer the log4j2 configs can use it
> by
> > > > > setting `export
> > > > >
> > >
> KAFKA_LOG4J_OPTS="-Dlog4j.configurationFile={log4j2-config-file-path}"`.
> > > > > Whenever we change the default logging format, we must don't forget
> > to
> > > > > switch this functionality to the reverse, i.e., making log4j 1.x
> > format
> > > > > available as an opt-in.
> > > > >
> > > > > I am so concerned about the community's opinion when would be
> > adequate
> > > to
> > > > > make the log4j2 config as default.
> > > > >
> > > > > Thanks,
> > > > > Dongjin
> > > > >
> > > > > +1. As a note, I have an almost-completed implementation of log4j2
> > > > > equivalent for the log4j-appender. I think it would be great if
> this
> > > > > feature can be provided with changing the default logging config
> > > format.
> > > > >
> > > > > On Wed, Sep 16, 2020 at 11:49 PM Ismael Juma <ismael@juma.me.uk>
> > > wrote:
> > > > >
> > > > >> Thanks for the KIP, Dongjin. Have we considered switching to
the
> > > log4j2
> > > > >> logging config format by default and providing a mechanism to
use
> > the
> > > > old
> > > > >> format? It is likely that we will release 3.0 as the release
after
> > > 2.7,
> > > > so
> > > > >> it would provide a good opportunity to move on from the legacy
> > config
> > > > >> format. The other option is to stick with the old format for
3.0
> and
> > > > >> migrate to the new format in 4.0.
> > > > >>
> > > > >> Ismael
> > > > >>
> > > > >> On Wed, Aug 5, 2020 at 7:45 AM Dongjin Lee <dongjin@apache.org>
> > > wrote:
> > > > >>
> > > > >> > Hi, Kafka dev,
> > > > >> >
> > > > >> > I hope to initiate the discussion of KIP-653, upgrading
log4j to
> > > > log4j2.
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >>
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-653%3A+Upgrade+log4j+to+log4j2
> > > > >> >
> > > > >> > All kinds of feedbacks are greatly appreciated!
> > > > >> >
> > > > >> > Best,
> > > > >> > Dongjin
> > > > >> >
> > > > >> > --
> > > > >> > *Dongjin Lee*
> > > > >> >
> > > > >> > *A hitchhiker in the mathematical world.*
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> >
> > > > >> > *github:  <http://goog_969573159/>github.com/dongjinleekr
> > > > >> > <https://github.com/dongjinleekr>keybase:
> > > > >> https://keybase.io/dongjinleekr
> > > > >> > <https://keybase.io/dongjinleekr>linkedin:
> > > > >> kr.linkedin.com/in/dongjinleekr
> > > > >> > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > > >> > speakerdeck.com/dongjin
> > > > >> > <https://speakerdeck.com/dongjin>*
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > > > --
> > > > > *Dongjin Lee*
> > > > >
> > > > > *A hitchhiker in the mathematical world.*
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > *github:  <http://goog_969573159/>github.com/dongjinleekr
> > > > > <https://github.com/dongjinleekr>keybase:
> > > > https://keybase.io/dongjinleekr
> > > > > <https://keybase.io/dongjinleekr>linkedin:
> > > > kr.linkedin.com/in/dongjinleekr
> > > > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > > speakerdeck.com/dongjin
> > > > > <https://speakerdeck.com/dongjin>*
> > > > >
> > > >
> > > >
> > > > --
> > > > *Dongjin Lee*
> > > >
> > > > *A hitchhiker in the mathematical world.*
> > > >
> > > >
> > > >
> > > >
> > > > *github:  <http://goog_969573159/>github.com/dongjinleekr
> > > > <https://github.com/dongjinleekr>keybase:
> > > https://keybase.io/dongjinleekr
> > > > <https://keybase.io/dongjinleekr>linkedin:
> > > kr.linkedin.com/in/dongjinleekr
> > > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > > speakerdeck.com/dongjin
> > > > <https://speakerdeck.com/dongjin>*
> > > >
> > >
> >
> >
> > --
> > *Dongjin Lee*
> >
> > *A hitchhiker in the mathematical world.*
> >
> >
> >
> >
> > *github:  <http://goog_969573159/>github.com/dongjinleekr
> > <https://github.com/dongjinleekr>keybase:
> https://keybase.io/dongjinleekr
> > <https://keybase.io/dongjinleekr>linkedin:
> kr.linkedin.com/in/dongjinleekr
> > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > speakerdeck.com/dongjin
> > <https://speakerdeck.com/dongjin>*
> >
>


-- 
*Dongjin Lee*

*A hitchhiker in the mathematical world.*




*github:  <http://goog_969573159/>github.com/dongjinleekr
<https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
<https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
<https://kr.linkedin.com/in/dongjinleekr>speakerdeck: speakerdeck.com/dongjin
<https://speakerdeck.com/dongjin>*

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message