Skip to content

KAFKA-18483: Disable Log4jController and Loggers if Log4j Core absent#18496

Merged
chia7712 merged 9 commits intoapache:trunkfrom
ppkarwasz:feature/optional-log4j
Feb 6, 2025
Merged

KAFKA-18483: Disable Log4jController and Loggers if Log4j Core absent#18496
chia7712 merged 9 commits intoapache:trunkfrom
ppkarwasz:feature/optional-log4j

Conversation

@ppkarwasz
Copy link
Copy Markdown
Contributor

If Log4j Core is absent, most calls to Log4jController and Loggers will end up with a NoClassDefFoundError.

This changeset:

  • Profits from the major version bump to rename k.util.Log4jController to LoggingController.
  • Removes o.a.l.l.Level from the signature of public methods of o.a.k.connect.runtime.Loggers and replaces it with String.
  • Provides an additional no-op implementation of k.util.LoggingController and o.a.k.connect.runtime.Loggers: if Log4j Core is not present on the runtime classpath the no-op implementation will be used.

Committer Checklist (excluded from commit message)

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

@chia7712
Copy link
Copy Markdown
Member

@ppkarwasz could you please fix the conflicts? I will get a quick review after the conflicts get fixed.

@github-actions
Copy link
Copy Markdown

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

@ppkarwasz
Copy link
Copy Markdown
Contributor Author

@ppkarwasz could you please fix the conflicts? I will get a quick review after the conflicts get fixed.

Sure, I'll fix the conflicts and check out the failing tests today.

…sent

If Log4j Core is absent, most calls to `Log4jController` and `Loggers` will end up with a `NoClassDefFoundError`.

This changeset:

- Profits from the major version bump to rename `k.util.Log4jController` to `LoggingController`.
- Removes `o.a.l.l.Level` from the signature of public methods of `o.a.k.connect.runtime.Loggers` and replaces it with `String`.
- Provides an additional no-op implementation of `k.util.LoggingController` and `o.a.k.connect.runtime.Loggers`: if Log4j Core is not present on the runtime classpath the no-op implementation will be used.
@ppkarwasz ppkarwasz force-pushed the feature/optional-log4j branch from b6a1610 to 9c36ec8 Compare January 21, 2025 09:00
@github-actions
Copy link
Copy Markdown

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

@ppkarwasz
Copy link
Copy Markdown
Contributor Author

Note: Before submitting the PR I checked it with ./gradlew test using JDK 17. I am not sure why it fails on the CI.

@chia7712
Copy link
Copy Markdown
Member

Before submitting the PR I checked it with ./gradlew test using JDK 17. I am not sure why it fails on the CI.

there are some flaky test in trunk branch. I feel they are unrelated to this PR :)

@mimaison
Copy link
Copy Markdown
Member

I hit the test failure locally when on your branch:

./gradlew core:test --tests "*LoggingTest"

> Task :core:test

Gradle Test Run :core:test > Gradle Test Executor 10 > LoggingTest > testLoggerLevelIsResolved() PASSED
kafka.utils.LoggingTest.testLog4jControllerIsRegistered() failed, log available in /Users/mickael/github/kafka/core/build/reports/testOutput/kafka.utils.LoggingTest.testLog4jControllerIsRegistered().test.stdout

Gradle Test Run :core:test > Gradle Test Executor 10 > LoggingTest > testLog4jControllerIsRegistered() FAILED
    org.opentest4j.AssertionFailedError: kafka.utils.Log4jController is not registered ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
        at app//kafka.utils.LoggingTest.testLog4jControllerIsRegistered(LoggingTest.scala:42)

Gradle Test Run :core:test > Gradle Test Executor 10 > LoggingTest > testTypeOfGetLoggers() PASSED

Gradle Test Run :core:test > Gradle Test Executor 10 > LoggingTest > testLogName() PASSED

Gradle Test Run :core:test > Gradle Test Executor 10 > LoggingTest > testLogNameOverride() PASSED

5 tests completed, 1 failed

@ppkarwasz
Copy link
Copy Markdown
Contributor Author

I hit the test failure locally when on your branch:

./gradlew core:test --tests "*LoggingTest"

Thanks somehow Gradle didn't mark the build as failed, even if this test didn't work. Fixed in b382950

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppkarwasz thanks for the patch! Sorry for the late review, I was busy with the Lunar New Year festivities :(

Comment thread core/src/main/scala/kafka/utils/Logging.scala Outdated
Comment thread core/src/main/scala/kafka/utils/LoggingController.scala Outdated
Comment thread core/src/main/scala/kafka/utils/LoggingController.scala
Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java Outdated
@ppkarwasz ppkarwasz requested a review from chia7712 January 27, 2025 20:28
@github-actions github-actions bot removed triage PRs from the community needs-attention labels Jan 28, 2025
@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Feb 6, 2025

@ppkarwasz Could you please fix the conflicts ?

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Test the patch on my local with JMC and changing the logger levels works well.

@chia7712 chia7712 merged commit 6665712 into apache:trunk Feb 6, 2025
@ppkarwasz ppkarwasz deleted the feature/optional-log4j branch February 6, 2025 16:05
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
…ent (apache#18496)

If Log4j Core is absent, most calls to Log4jController and Loggers will end up with a NoClassDefFoundError.

This changeset:

- Profits from the major version bump to rename k.util.Log4jController to LoggingController.
- Removes o.a.l.l.Level from the signature of public methods of o.a.k.connect.runtime.Loggers and replaces it with String.
- Provides an additional no-op implementation of k.util.LoggingController and o.a.k.connect.runtime.Loggers: if Log4j Core is not present on the runtime classpath the no-op implementation will be used.

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
…ent (apache#18496)

If Log4j Core is absent, most calls to Log4jController and Loggers will end up with a NoClassDefFoundError.

This changeset:

- Profits from the major version bump to rename k.util.Log4jController to LoggingController.
- Removes o.a.l.l.Level from the signature of public methods of o.a.k.connect.runtime.Loggers and replaces it with String.
- Provides an additional no-op implementation of k.util.LoggingController and o.a.k.connect.runtime.Loggers: if Log4j Core is not present on the runtime classpath the no-op implementation will be used.

Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

connect core Kafka Broker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants