KAFKA-18466: Remove log4j-1.2-api from runtime scope while keeping it in distribution package

[frankvicky]

JIRA: KAFKA-18466

This PR is a patch for #17373 (comment)

Committer Checklist (excluded from commit message)

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

[chia7712]

@frankvicky please build the distribution and Pom file and then check the content, thanks!

[frankvicky]

POM file from local maven:

Content of libs after releaseTarGz:

[ppkarwasz]

Similarly to apache/eventmesh#4700, I think that:

• Not only log4j-1.2-api, but also log4j-slf4j-impl should be removed from the runtime dependencies of all artifacts.
• log4j-core should be an optional runtime dependency in kafka_2_13 and connect-runtime.
• I am not sure if log4j-api should be a direct dependency at all: you only use it in kafka_2_13 and connect-runtime, because you call log4j-core methods that have log4j-api types in the signature. IMHO it is OK to remove log4j-api and rely on the fact the log4j-core depends on log4j-api.

[chia7712]

I totally agree @ppkarwasz's suggestion. Additionally, jackson-dataformat-yaml can be removed from runtime dependency too. Maybe log4j-core and log4j-api can be moved to compileOnly and then include them into distribution?

[ppkarwasz]

Additionally, jackson-dataformat-yaml can be removed from runtime dependency too.

+1

Maybe log4j-core and log4j-api can be moved to compileOnly and then include them into distribution?

Gradle apparently has a feature for that, see Optional dependencies are not optional. With a snippet like:

java {
    registerFeature('loggingConfig') {
        usingSourceSet(sourceSets.main)
    }
}

it should be possible to:

• declare log4j-core as a optional Maven dependency with a scope of runtime.
• Gradle users will still be able to make import the transitive dependency using:

  implementation("org.apache.kafka:kafka_2_13") {
    capabilities {
      requireCapability("org.apache.kafka:kafka_2_13-logging-config")
    }
  }

[frankvicky]

I have adopted the feature variant at the latest commit.

generated pom:

Content of libs after releaseTarGz:

[frankvicky]

Hi @ppkarwasz,

Should we make log4j-slf4j-impl an optional dependency?

I'm running the kafka e2e on my local machine, it would fail with following log:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

It seems we still need log4j-slf4j-impl at runtime. WDYT ?

[chia7712]

It seems we still need log4j-slf4j-impl at runtime. WDYT ?

please add the log4jRuntimeLibs to copyDependantLibs too. Our docker e2e depends on systemTestLibs task by default, and the task copies the dependencies to dependant-testlibs folder

[chia7712]

I think only core and connect-runtime needs to add log4j2Libs to compileOnly scope.

[frankvicky]

I have run a few local tests to see if log4j works.
Quickstart:

connect-standalone:

e2e (connect_test.py):

[chia7712]

LGTM

test the latest commit on my local.

chia7712@chia7712-ubuntu:~$ cat ~/.m2/repository/org/apache/kafka/kafka_2.13/4.1.0-SNAPSHOT/kafka_2.13-4.1.0-SNAPSHOT.pom | grep log4j
chia7712@chia7712-ubuntu:~$ 

chia7712@chia7712-ubuntu:~$ cat ~/.m2/repository/org/apache/kafka/kafka-clients/4.1.0-SNAPSHOT/kafka-clients-4.1.0-SNAPSHOT.pom | grep log4j
chia7712@chia7712-ubuntu:~$ cat ~/.m2/repository/org/apache/kafka/kafka-clients/4.1.0-SNAPSHOT/kafka-clients-4.1.0-SNAPSHOT.pom | grep slf4j
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>

chia7712@chia7712-ubuntu:~$ cat ~/.m2/repository/org/apache/kafka/kafka-streams/4.1.0-SNAPSHOT/kafka-streams-4.1.0-SNAPSHOT.pom | grep log4j
chia7712@chia7712-ubuntu:~$ cat ~/.m2/repository/org/apache/kafka/kafka-streams/4.1.0-SNAPSHOT/kafka-streams-4.1.0-SNAPSHOT.pom | grep slf4j
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>

[chia7712]

@ppkarwasz we don't use the Gradle’s feature variants in this PR due to following reasons.

1. We do not expect users to include the optional dependencies. Instead, users should explicitly add their preferred SLF4j provider (including the specific version) to their project dependencies. If they also include a Log4j provider, they will need to resolve any potential conflicts.
2. Utilizing configurations.releaseOnly can effectively add the necessary Log4j 2 dependencies to the classpath for end-to-end tests and scripts.

I will merge this PR to fix the issue reported by @trnguyencflt, but please feel free to leave suggestions as follow-up, thanks!

[ppkarwasz]

@ppkarwasz we don't use the Gradle’s feature variants in this PR due to following reasons.

1. We do not expect users to include the optional dependencies. Instead, users should explicitly add their preferred SLF4j provider (including the specific version) to their project dependencies. If they also include a Log4j provider, they will need to resolve any potential conflicts.

2. Utilizing `configurations.releaseOnly` can effectively add the necessary Log4j 2 dependencies to the classpath for end-to-end tests and scripts.

The PR looks good to me now. For a POM file having log4j-core as an optional runtime dependency or not having it at all is pretty much a matter of taste.

[ppkarwasz]

@chia7712,

I have created #18496 to protect the two classes that use Log4j Core (k.util.Log4jController and o.a.k.connect.runtime.Loggers) from throwing a linkage error if Log4j Core is not present on the runtime classpath.

[trnguyencflt]

thanks all for the quick fix 👍