KAFKA-9366: Upgrade log4j to log4j2 #17373

frankvicky · 2024-10-04T15:43:35Z

JIRA: KAFKA-9366

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

milestone 1

frankvicky · 2024-10-04T16:43:26Z

This is the initial version. I'd like to run it on CI first.

mumrah

Thanks for tackling this @frankvicky.

Is it really necessary to rename the log4j config files? The KIP mentions a system property that allows us to load log4j.properties files https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158870552#KIP653:Upgradelog4jtolog4j2-Compatibility,Deprecation,andMigrationPlan

I could understand migrating our internal log4j configs for tests and such, but I think we should avoid forcing a change in our production configs.

I also see that this page mentions a log4j to log4j2 bridge https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html. Would that help us at all?

mumrah · 2024-10-04T17:36:06Z

build.gradle

+    testRuntimeOnly libs.slf4jLog4j2
+    testRuntimeOnly libs.junitPlatformLanucher


These (and other similar testRuntimeOnly) should be put into the runtimeTestLibs definition

mumrah · 2024-10-04T17:36:26Z

build.gradle

@@ -2596,6 +2626,7 @@ project(':streams') {
    implementation libs.slf4jApi
    implementation libs.jacksonAnnotations
    implementation libs.jacksonDatabind
+    implementation libs.bndlib


What's this dependency for?

Hi @mumrah
I add this to fix the warning during build:

/home/frankvicky/.gradle/caches/modules-2/files-2.1/org.apache.logging.log4j/log4j-api/2.24.1/7ebeb12c20606373005af4232cd0ecca72613dda/log4j-api-2.24.1.jar(/org/apache/logging/log4j/Level.class): warning: Cannot find annotation method 'value()' in type 'BaselineIgnore': class file for aQute.bnd.annotation.baseline.BaselineIgnore not found

I'm not convinced we need that dependency. Also it seems to complain about an annotation so at least we should not need it at runtime, so we should not include it in our distribution package. Currently it's included in the artifact generated by releaseTarGz.

Fair enough, I will try to solve this one.

The BND annotations are intentionally in the provided Maven scope of all Log4j artifacts, so that these annotations with CLASS retention do not end up in the runtime classpath. You can do the same and add them as compileOnly in Gradle.

The compiler warnings should disappear once JDK-8342833 is fixed.
Untile then we will remove the outdated ones (see apache/logging-log4j2#3133) in the next Log4j release, which should remove the warning on Level.

Thanks for the information.
I have already changed its scope to compile time.
PTAL 😄

mumrah · 2024-10-04T17:47:01Z

bin/kafka-server-start.sh

@@ -22,7 +22,7 @@ fi
 base_dir=$(dirname $0)

 if [ "x$KAFKA_LOG4J_OPTS" = "x" ]; then
-    export KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/log4j.properties"


This could break some existing Kafka installations. If users are extracting in place or copying previous config files to a new installation directory, they will be expecting the log4j.properties to still work.

The switch from the legacy to the new configuration format can be based on the presence of specific files:

if [ -f "$base_dir/../config/log4j.properties" ]; then echo DEPRECATED: Using Log4j 1.x configuration file \$KAFKA_HOME/config/log4j.properties >&2 echo To use a Log4j 2.x configuration, create a \$KAFKA_HOME/config/log4j2.xml file and remove the Log4j 1.x configration. >&2 echo See https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#Log4j2ConfigurationFormat for details about Log4j configuration file migration. >&2 export KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/log4j.properties" elif [ -f "$base_dir/../config/log4j2.xml" ]; then export KAFKA_LOG4J_OPTS="-Dlog4j2.configurationFile=$base_dir/../config/log4j2.xml" fi

frankvicky · 2024-10-06T03:32:10Z

Hello @mumrah
Thanks for your feedback. Unfortunately, I barely missed the KIP for some reason, but I'll take a look and adjust the PR accordingly. 😺

revert unnecessary change

chia7712 · 2024-11-19T14:00:39Z

When you merge this PR, please don't omit mentioning my credit. 🙇‍♂️

will roger that :)

chia7712

@frankvicky thanks for this patch. please check my latest comments to fix e2e

chia7712 · 2024-11-19T16:23:18Z

tests/kafkatest/services/connect.py

@@ -364,7 +364,7 @@ def start_node(self, node, **kwargs):
        if self.external_config_template_func:
            node.account.create_file(self.EXTERNAL_CONFIGS_FILE, self.external_config_template_func(node))
        node.account.create_file(self.CONFIG_FILE, self.config_template_func(node))
-        node.account.create_file(self.LOG4J_CONFIG_FILE, self.render('connect_log4j.properties', log_file=self.LOG_FILE))
+        node.account.create_file(self.LOG4J_CONFIG_FILE, self.render('connect_log4j2.properties', log_file=self.LOG_FILE))


in the e2e we could run kafak on different version, so we must check the version before applying the config file.

it seems connect e2e does not run different version for workers, so you can just change them to connect_log4j2.yaml. However, please change the -Dlog4j.configuration to -Dlog4j2.configurationFile

chia7712 · 2024-11-19T16:23:22Z

tests/kafkatest/services/connect.py

@@ -421,7 +421,7 @@ def start_node(self, node, **kwargs):
        if self.external_config_template_func:
            node.account.create_file(self.EXTERNAL_CONFIGS_FILE, self.external_config_template_func(node))
        node.account.create_file(self.CONFIG_FILE, self.config_template_func(node))
-        node.account.create_file(self.LOG4J_CONFIG_FILE, self.render('connect_log4j.properties', log_file=self.LOG_FILE))
+        node.account.create_file(self.LOG4J_CONFIG_FILE, self.render('connect_log4j2.properties', log_file=self.LOG_FILE))


chia7712 · 2024-11-19T16:23:25Z

tests/kafkatest/services/kafka/kafka.py

@@ -874,7 +874,7 @@ def start_node(self, node, timeout_sec=60, **kwargs):
        self.logger.info("kafka.properties:")
        self.logger.info(prop_file)
        node.account.create_file(KafkaService.CONFIG_FILE, prop_file)
-        node.account.create_file(self.LOG4J_CONFIG, self.render('log4j.properties', log_dir=KafkaService.OPERATIONAL_LOG_DIR))
+        node.account.create_file(self.LOG4J_CONFIG, self.render('log4j2.properties', log_dir=KafkaService.OPERATIONAL_LOG_DIR))


Please noted that the previous version should use -Dlog4j.configuration and trunk version should use -Dlog4j2.configurationFile

It seems that tons of code needs to apply this change.
I will prepare it asap.

mimaison · 2024-11-19T16:36:59Z

When you merge this PR, please don't omit mentioning my credit. 🙇‍♂️

Yes you totally deserve being marked as a co-author. Thanks!

mimaison · 2024-11-20T13:32:52Z

I'm having issues with the latest code (f3a68e1).

$ bin/kafka-server-start.sh config/kraft/reconfig-server.properties
[0.002s][error][logging] Error opening log file '/Users/mickael/github/kafka/core/build/distributions/kafka_2.13-4.0.0-SNAPSHOT/bin/../logs/kafkaServer-gc.log': No such file or directory
[0.002s][error][logging] Initialization of output 'file=/Users/mickael/github/kafka/core/build/distributions/kafka_2.13-4.0.0-SNAPSHOT/bin/../logs/kafkaServer-gc.log' using options 'filecount=10,filesize=100M' failed.
Invalid -Xlog option '-Xlog:gc*:file=/Users/mickael/github/kafka/core/build/distributions/kafka_2.13-4.0.0-SNAPSHOT/bin/../logs/kafkaServer-gc.log:time,tags:filecount=10,filesize=100M', see error log for details.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

Running trunk the logs folder is automatically created. If I create the folder manually, then it seems to log at WARN level by default and I only get a single log line printed while my broker is running:

$ bin/kafka-server-start.sh config/kraft/reconfig-server.properties
[2024-11-20 14:33:39,425] WARN [QuorumController id=1] Performing controller activation. The metadata log appears to be empty. Appending 2 bootstrap record(s) in metadata transaction at metadata.version 4.0-IV0 from bootstrap source 'the binary bootstrap metadata file: /tmp/kraft-combined-logs/bootstrap.checkpoint'. Setting the ZK migration state to NONE since this is a de-novo KRaft cluster. (org.apache.kafka.controller.QuorumController)

chia7712

@frankvicky please double check the yaml format. I leave some comments about the incorrect format.

chia7712 · 2024-11-20T14:05:39Z

tests/kafkatest/services/templates/connect_log4j2.yaml

+        immediateFlush: true
+        PatternLayout:
+          pattern: "${logPattern}"
+        TimeBasedTriggeringPolicy:


Policies: TimeBasedTriggeringPolicy: modulate: true interval: 1

chia7712 · 2024-11-20T14:05:59Z

tests/kafkatest/services/templates/connect_log4j2.yaml

+
+    RollingFile:
+      - name: FILE
+        fileName: {{ log_file }}


please add filePattern

ppkarwasz · 2024-11-20T15:21:56Z

config/log4j2.yaml

+        Polices:
+          TimeBasedTriggeringPolicy:
+            modulate: true
+            interval: 1


Since there is only one triggering policy, there is no need to wrap it in a Policies wrapper.

BTW: there is a typo in the plugin name: it should be Policies, instead of Polices.

Thanks for information 😄

frankvicky and others added 2 commits October 4, 2024 23:31

WIP

4cc09d9

milestone 1

Merge branch 'trunk' into KAFKA-9366

d71bbdf

github-actions bot added streams core Kafka Broker tools connect kraft mirror-maker-2 storage Pull requests that target the storage module build Gradle build or GitHub Actions docker Official Docker image clients labels Oct 4, 2024

frankvicky force-pushed the KAFKA-9366 branch from 093c8d6 to 0ecc326 Compare October 4, 2024 16:37

frankvicky marked this pull request as ready for review October 4, 2024 16:42

frankvicky added the do-not-merge PRs that are only open temporarily and should not be merged label Oct 4, 2024

mumrah reviewed Oct 4, 2024

View reviewed changes

frankvicky marked this pull request as draft October 6, 2024 03:19

frankvicky added 2 commits October 10, 2024 21:53

KAFKA-9366: Upgrade log4j to log4j2

347388a

Merge branch 'trunk' into KAFKA-9366

9fc1203

frankvicky force-pushed the KAFKA-9366 branch from 0ecc326 to 9fc1203 Compare October 10, 2024 13:54

frankvicky marked this pull request as ready for review October 10, 2024 13:55

frankvicky force-pushed the KAFKA-9366 branch from 0c1e6bf to 4b722a5 Compare October 10, 2024 14:27

frankvicky marked this pull request as draft October 10, 2024 16:23

frankvicky added 2 commits October 11, 2024 19:53

KAFKA-9366: Cleanup

d12f311

Merge branch 'trunk' into KAFKA-9366

7200b43

frankvicky force-pushed the KAFKA-9366 branch from 4b722a5 to 7200b43 Compare October 11, 2024 11:53

frankvicky marked this pull request as ready for review October 11, 2024 11:54

revert unnecessary change

7419f5f

revert unnecessary change

KAFKA-9366: Address comments

5e1eaf5

frankvicky force-pushed the KAFKA-9366 branch from dcc2ac1 to 5e1eaf5 Compare November 19, 2024 14:10

chia7712 reviewed Nov 19, 2024

View reviewed changes

KAFKA-9366: migration to yaml

a1c2294

frankvicky force-pushed the KAFKA-9366 branch from 8505f70 to a1c2294 Compare November 20, 2024 09:48

frankvicky added 2 commits November 20, 2024 19:40

KAFKA-9366: Update script and readme

7672fa5

KAFKA-9366: update e2e

f3a68e1

frankvicky force-pushed the KAFKA-9366 branch from da3045d to f3a68e1 Compare November 20, 2024 13:20

chia7712 reviewed Nov 20, 2024

View reviewed changes

KAFKA-9366: fix yaml

9343fc3

ppkarwasz reviewed Nov 20, 2024

View reviewed changes

frankvicky added 2 commits November 20, 2024 23:31

KAFKA-9366: fix typo and compact the single policy

5761828

Merge branch 'trunk' into KAFKA-9366

723e87c

frankvicky force-pushed the KAFKA-9366 branch from a85ddc0 to bbed45a Compare November 21, 2024 06:35

KAFKA-9366: update e2e

40a80f4

frankvicky force-pushed the KAFKA-9366 branch from bbed45a to 40a80f4 Compare November 21, 2024 07:58

frankvicky added 5 commits November 21, 2024 16:04

KAFKA-9366: update script

a6fa0ac

KAFKA-9366: Fix typo

17cc7fc

KAFKA-9366: update util.py

8fa5a73

KAFKA-9366: Fix typo

1d06e84

KAFKA-9366: update e2e log4j2.yaml

e488b0a

frankvicky force-pushed the KAFKA-9366 branch from e6a2fd8 to e488b0a Compare November 22, 2024 07:57

frankvicky added 2 commits November 22, 2024 16:09

Merge branch 'trunk' into KAFKA-9366

89b2168

KAFKA-9366: Fix import-control

8113166

github-actions bot added the consumer label Nov 22, 2024

frankvicky added 2 commits November 22, 2024 17:43

KAFKA-9366: update e2e log4j2.yaml

2b749e8

KAFKA-9366: revert uncessary changes

65e906e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-9366: Upgrade log4j to log4j2 #17373

KAFKA-9366: Upgrade log4j to log4j2 #17373

frankvicky commented Oct 4, 2024

frankvicky commented Oct 4, 2024

mumrah left a comment

mumrah Oct 4, 2024

mumrah Oct 4, 2024

frankvicky Oct 6, 2024

mimaison Oct 23, 2024

frankvicky Oct 24, 2024

ppkarwasz Oct 28, 2024

frankvicky Oct 29, 2024

mumrah Oct 4, 2024

ppkarwasz Oct 28, 2024

frankvicky commented Oct 6, 2024

chia7712 commented Nov 19, 2024

chia7712 left a comment

chia7712 Nov 19, 2024

chia7712 Nov 20, 2024

chia7712 Nov 19, 2024

chia7712 Nov 19, 2024

chia7712 Nov 20, 2024

frankvicky Nov 20, 2024

mimaison commented Nov 19, 2024

mimaison commented Nov 20, 2024 •

edited

Loading

chia7712 left a comment

chia7712 Nov 20, 2024

chia7712 Nov 20, 2024

ppkarwasz Nov 20, 2024

frankvicky Nov 20, 2024

		testRuntimeOnly libs.slf4jLog4j2
		testRuntimeOnly libs.junitPlatformLanucher

KAFKA-9366: Upgrade log4j to log4j2 #17373

Are you sure you want to change the base?

KAFKA-9366: Upgrade log4j to log4j2 #17373

Conversation

frankvicky commented Oct 4, 2024

Committer Checklist (excluded from commit message)

frankvicky commented Oct 4, 2024

mumrah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankvicky commented Oct 6, 2024

chia7712 commented Nov 19, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison commented Nov 19, 2024

mimaison commented Nov 20, 2024 • edited Loading

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison commented Nov 20, 2024 •

edited

Loading