Skip to content

[dist][runtime] Use Flink-provided Log4j dependencies - #954

Merged
wenjin272 merged 1 commit into
apache:mainfrom
rosemarYuan:fix/log4j-packaging
Aug 4, 2026
Merged

[dist][runtime] Use Flink-provided Log4j dependencies#954
wenjin272 merged 1 commit into
apache:mainfrom
rosemarYuan:fix/log4j-packaging

Conversation

@rosemarYuan

Copy link
Copy Markdown
Contributor

Linked issue: #953

Purpose of change

The shaded Dist artifacts currently receive Log4j and SLF4J dependencies transitively from Plan, even though the default logging setup of the supported Flink distributions already provides Log4j2 and the SLF4J binding for JobManager and TaskManager processes.

This can place different Log4j API and Core versions on the same runtime classpath and cause linkage errors such as NoSuchMethodError.

This PR makes the logging dependency and packaging boundary explicit:

  • Keep slf4j-api as a normal Plan dependency so standalone Plan tools remain usable, but remove log4j-core and log4j-slf4j-impl from Plan.
  • Keep Runtime's Log4j dependencies as provided and align the compile-time Log4j version with 2.24.3, the version used by the oldest supported Flink distributions.
  • Exclude Log4j and SLF4J artifacts, classes, service metadata, and log4j2.* configuration files from all shaded Dist artifacts.
  • Add test-scoped Log4j dependencies for the E2E test JVM and align them with the selected Flink version: 2.24.3 for Flink 1.20 through 2.2 and 2.25.3 for Flink 2.3.
  • Update META-INF/NOTICE so it no longer lists Log4j or SLF4J as bundled dependencies.

After this change, Flink Agents uses the Log4j2 implementation provided by the default logging setup of the selected supported Flink distribution and no longer packages a second logging stack.

Tests

  • Built all common, full, and thin Dist artifacts for the supported Flink versions.
  • Verified that the generated Dist JARs contain no Log4j or SLF4J classes, Log4j service metadata, or log4j2.* configuration files.
  • Verified that the generated NOTICE files match the updated artifact contents.

Full E2E and clean Flink deployment tests were not run locally.

API

No public API changes.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Aug 3, 2026
@wenjin272

Copy link
Copy Markdown
Contributor

Could you help review this pr @GreatEugenius?

Comment thread plan/pom.xml

@GreatEugenius GreatEugenius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi, @rosemarYuan. Thanks for the PR. Overall, it looks good to me. I have just one minor suggestion.

@rosemarYuan

Copy link
Copy Markdown
Contributor Author

Hi, @rosemarYuan. Thanks for the PR. Overall, it looks good to me. I have just one minor suggestion.

Thanks for the suggestion! I applied it, but it turns out this change breaks the cross-language E2E tests — the CI failure is NoClassDefFoundError: org/slf4j/LoggerFactory.The reason is that in cross-language tests, Python spawns a Java subprocess running the Plan tool directly (not inside a Flink cluster). When slf4j-api is marked as provided, it's excluded from the Plan module's transitive dependencies, so the subprocess's classpath doesn't include it. Unlike IT tests (where Flink's lib/ provides SLF4J) or unit tests (where Maven automatically adds provided deps to the test classpath), the cross-language test environment has no external provider for this dependency.This is why I originally kept slf4j-api as a compile-scope dependency in Plan — standalone Plan tool usage (including being invoked from Python) requires it to be bundled. The Dist shade plugin already excludes slf4j-api from the fat jar, so there's no duplication risk in the Flink deployment scenario.

I'll revert this commit to fix the CI.

@rosemarYuan
rosemarYuan force-pushed the fix/log4j-packaging branch from 7e7a1cd to 35d8d21 Compare August 4, 2026 07:59

@wenjin272 wenjin272 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for picking this up @rosemarYuan.

@wenjin272
wenjin272 merged commit bf31f52 into apache:main Aug 4, 2026
81 of 82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants