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 Tue, 29 Sep 2020 21:43:44 GMT
Hi All,

As you can see in the PR, I eliminated all compatibility breaks caused by
the root logger name change between log4j and log4j2. (i.e., "root" → "")
Plus, I rebased the PR onto the latest trunk, with migrating raft module
into log4j2.

Please have a look. And please note that now we have limited time only.

Thanks,
Dongjin

On Tue, Sep 29, 2020 at 1:07 AM Dongjin Lee <dongjin@apache.org> wrote:

> > 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>*
>


-- 
*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