Hi Mickael and Luke, Nice catch! Sure, I just updated the KIP document and the PR. As you can see in '8. embedded zookeeper' section, the embedded zookeeper launcher script will show that it does not support the Log4j MBean registration feature anymore. (And then, zookeeper's `ManagedUtil.isLog4jJmxEnabled` will display "Log4j 1.2 jmx support not found; jmx disabled.")
Thanks, Dongjin On Sat, Jan 29, 2022 at 5:00 PM Luke Chen <show...@gmail.com> wrote: > Hi Mickael, > > Nice catch! > I agree that we should mention the zookeeper log4j dependency things in the > KIP or upgrade/release notes. > > Thank you. > Luke > > On Fri, Jan 28, 2022 at 10:35 PM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > Hi Dongjin, > > > > Before starting to review the PR, I was doing a bit of research of the > > topic and I noticed ZooKeeper actually depends on log4j1. > > > > In zookeeper-server, by default a few JMX beans are registered for > > log4j1. When starting ZooKeeper you get a log line like: > > INFO Log4j 1.2 jmx support found and enabled. > > (org.apache.zookeeper.jmx.ManagedUtil) > > > > See > > > https://github.com/apache/zookeeper/blob/branch-3.6/zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java > > In case, log4j1 can't be found, this feature is disabled. It can also > > be disabled using the "zookeeper.jmx.log4j.disable" system property. > > But otherwise, it's enabled by default. > > > > People depending on this feature can always run a proper ZooKeeper > > distribution, instead of the ZooKeeper release we bundle with Kafka, > > or even manually add the log4j1 jars to their PATH. > > So I don't think it's a problem but I think we probably want to > > mention it in the KIP or in the upgrade/release notes at least. > > > > Thanks, > > Mickael > > > > On Thu, Oct 22, 2020 at 4:27 AM Dongjin Lee <dong...@apache.org> wrote: > > > > > > Hi All, > > > > > > As KIP-653 is now accepted, I hope to start a discussion about its > > release > > > plan. > > > > > > As all of you know, this proposal has the possibility of a > compatibility > > > break, for the changes between log4j and log4j2. To minimize the > > unexpected > > > consequences, I propose to provide a preview release. > > > > > > 1. Keep the current PR to the latest trunk, aiming for the next release > > > after 2.7.0. > > > 2. For 2.7.0, release a customized version with the log4j2 feature for > > > preview. The users who want to test this feature can download and use > > this > > > one instead of the official release. > > > 3. For 2.7.0 and 2.6.1, provide a patch for the log4j2 feature. The > users > > > who want to test this feature with their customized Kafka variant can > use > > > this one.[^1] > > > > > > For 2 and 3, I plan to release them on my apache FTP with documentation > > and > > > announce it in the dev and user mailing list. > > > > > > If you have any proposals or ideas, don't hesitate to give me a reply. > > > > > > Thanks, > > > Dongjin > > > > > > [^1]: CCed for in-house variant maintainers. > > > > > > On Wed, Sep 30, 2020 at 6:43 AM Dongjin Lee <dong...@apache.org> > wrote: > > > > > > > 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>* > > > > > > > > > > > > > -- > > > *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>*
