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 <dong...@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 <tbent...@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 <dong...@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 <tbent...@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 <dong...@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 <dong...@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 <ism...@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 <dong...@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>*
