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