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