Hi Mickael, I greatly appreciate you for reading the proposal so carefully! I wrote it quite a while ago and rechecked it today.
> Is the KIP proposing to replace the existing log4-appender or simply add a new one for log4j2? Reading the KIP and with its current title, it's not entirely explicit. Oh, After re-reading it, I realized that this is not clear. Let me clarify; 1. Provide a lo4j2 equivalent of traditional log4j-appender, log4j2-appender. 2. Migrate the modules depending on log4j-appender (i.e., tools, trogdor, shell) into log4j2-appender, removing log4j-appender from dependencies. 3. Entirely remove log4j-appender from the project dependencies, along with log4j. I think log4j-appender may be published for every new release like before, but the committee should make a decision on the policy. > Under Rejected Alternative, the KIP states: "the Kafka appender provided by log4j2 community stores log message in the Record key". Looking at the code, it looks like the log message is stored in the Record value: https://github.com/apache/logging-log4j2/blob/master/log4j-kafka/src/main/java/org/apache/logging/log4j/kafka/appender/KafkaManager.java#L135 Am I missing something? It's totally my fault; I confused it with another appender. The compatibility problem in the logging-log4j2 Kafka appender is not the format but the configuration. logging-log4j2 Kafka appender supports `properties` configuration, which will be directly used to instantiate a Kafka producer. However, log4j-appender has been using non-producer config names like brokerList (=bootstrap.servers), requiredNumAcks (=acks). Instead, logging-log4j2 Kafka appender supports retryCount, sendEventTimestamp. On second thought, using logging-log4j2 Kafka appender internally and making log4j2-appender to focus on compatibility facade only would be a better approach; As I described above, the goal of this module is just keeping the backward-compatibility, and (as you pointed out) the current implementation has little value. Since org.apache.logging.log4j:log4j-core already includes Kafka appender, we can make use of the 'proven wheel' without adding more dependencies. I have not tried it yet, but I think it is well worth it. (One additional advantage of this approach is providing a bridge to the users who hope to move from/into logging-log4j2 Kafka appender.) > As the current log4j-appender is not even deprecated yet, in theory we can't remove it till Kafka 4. If we want to speed up the process, I wonder if the lack of documentation and a migration guide could help us. What do you think? In fact, this is what I am doing nowadays. While working with log4j-appender, I found that despite a lack of documentation, considerable users are already using it[^1][^2][^3][^4][^5]. So, I think providing a documentation to those who are already using log4j-appender is indispensable. It should include: - What is the difference between log4j-appender vs. log4j2-appender. - Which options are supported and deprecated. - Exemplar configurations that show how to migrate. Here is the summary: 1. The goal of this proposal is to replace the traditional log4j-appender for compatibility concerns. But log4j-appender may be published after the deprecation. 2. As of present, the description about logging-log4j2 Kafka appender is entirely wrong. The problem is interface compatibility, not record format. Focusing on the compatibility facade is a good approach. 3. A documentation focus on migration should be provided. If you have any questions or suggestions, don't hesitate to tell me. Thanks again for your comments! Best, Dongjin [^1]: https://docs.cloudera.com/csa/1.2.0/monitoring/topics/csa-kafka-logging.html [^2]: https://stackoverflow.com/questions/22034895/how-to-use-kafka-0-8-log4j-appender [^3]: https://stackoverflow.com/questions/32402405/delay-in-kafka-log4j-appender [^4]: https://stackoverflow.com/questions/32301129/kafka-log4j-appender-not-sending-messages [^5]: https://stackoverflow.com/questions/35628706/kafka-log4j-appender-0-9-does-not-work On Mon, Nov 8, 2021 at 9:04 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi Dongjin, > > Thanks for working on the update to log4j2, it's definitively > something we should complete. > I have a couple of comments: > > 1) Is the KIP proposing to replace the existing log4-appender or > simply add a new one for log4j2? Reading the KIP and with its current > title, it's not entirely explicit. For example I don't see a statement > under the proposed changes section. The PR seems to only add a new > appender but the KIP mentions we want to fully remove dependencies to > log4j. > > 2) Under Rejected Alternative, the KIP states: "the Kafka appender > provided by log4j2 community stores log message in the Record key". > Looking at the code, it looks like the log message is stored in the > Record value: > https://github.com/apache/logging-log4j2/blob/master/log4j-kafka/src/main/java/org/apache/logging/log4j/kafka/appender/KafkaManager.java#L135 > Am I missing something? > Comparing it with the proposed new appender, apart from their > configuration format (hence the backwards compatibility issues), they > both work pretty much the same way, so it's not clear it would add a > ton a value. > > At a glance, _I've not extensively looked at it_, it does not look > very hard to migrate to the appender from the logging team. I was > wondering if we should mention it in our documentation but I was not > able to find any references to the log4j-appender in the Kafka docs: > https://github.com/apache/kafka-site/search?q=KafkaLog4jAppender > > As the current log4j-appender is not even deprecated yet, in theory we > can't remove it till Kafka 4. If we want to speed up the process, I > wonder if the lack of documentation and a migration guide could help > us. What do you think? > > Thanks, > Mickael > > > > > On Fri, Jun 11, 2021 at 4:57 PM Boojapho O <booja...@gmail.com> wrote: > > > > Continuing to use log4j would leave several known security > vulnerabilities in Apache Kafka, including > https://nvd.nist.gov/vuln/detail/CVE-2019-17571. The Apache log4j team > will not fix this vulnerability and is urging an upgrade to log4j2. See > https://logging.apache.org/log4j/1.2/ for further information. > > > > This is desperately needed in Apache 3.0 to keep the software secure. > > > > On 2021/05/26 12:31:20, Dongjin Lee <dong...@apache.org> wrote: > > > CC'd the +1ers of KIP-653 with detailed context: > > > > > > When I submitted and got the approval of KIP-653: Upgrade log4j to > log4j2 > > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-653%3A+Upgrade+log4j+to+log4j2 > >, > > > I thought the log4j2-appender should not be the scope of the work. But > it > > > was wrong. > > > > > > Since the VerifiableLog4jAppender tool is built upon log4j-appender, > log4j > > > 1.x artifact will co-exist with log4j2 artifact in the classpath within > > > this scheme. Since the log4j 1.x code is not called anymore, I thought > it > > > is not problematic but actually, it was not - when I started to > provide a > > > preview of KIP-653 > > > <http://home.apache.org/~dongjin/post/apache-kafka-log4j2-support/>, > some > > > users reported that sometimes slf4j fails to find the appropriate > binding > > > within the classpath, resulting fail to append the log message. > > > > > > To resolve this problem, I subtly adjusted the scope of the work; I > > > excluded Tools and Trogdor from KIP-653 and extended KIP-719 to take > care > > > of them instead, along with providing log4j2-appender. It is why the > > > current WIP implementations include some classpath logic in the shell > > > script and *why KIP-653 only can't complete the log4j2 migration*. > > > > > > I hope you will check this proposal out. > > > > > > Best, > > > Dongjin > > > > > > On Tue, May 25, 2021 at 10:43 PM Dongjin Lee <dong...@apache.org> > wrote: > > > > > > > Bumping up the discussion thread. > > > > > > > > Recently, I updated the document of KIP-653: Upgrade log4j to log4j2 > > > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-653%3A+Upgrade+log4j+to+log4j2> > (accepted) > > > > and KIP-719: Add Log4J2 Appender > > > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-719%3A+Add+Log4J2+Appender> > (under > > > > discussion) reflecting the recent changes to our codebase. > Especially: > > > > > > > > 1. KIP-653 document > > > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-653%3A+Upgrade+log4j+to+log4j2> > now > > > > explains which modules will be migrated and why. > > > > 2. KIP-719 document > > > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-719%3A+Add+Log4J2+Appender> > now > > > > explains not only the log4j2-appender plan but also upgrading the > omitted > > > > modules in KIP-653 into log4j2. > > > > > > > > As you can see here, those two KIPs are the different parts of the > same > > > > problem. I believe the community will have a good grasp on why both > KIPs > > > > are best if released altogether. > > > > > > > > I will open the voting thread now, and please leave a vote if you are > > > > interested in this issue. > > > > > > > > Best, > > > > Dongjin > > > > > > > > On Tue, Mar 2, 2021 at 5:00 PM Dongjin Lee <dong...@apache.org> > wrote: > > > > > > > >> Hi Kafka dev, > > > >> > > > >> I would like to start the discussion of KIP-719: Add Log4J2 > Appender. > > > >> > > > >> > > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-719%3A+Add+Log4J2+Appender > > > >> > > > >> 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>*
