Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove log4j dependencies #367

Merged
merged 5 commits into from
Feb 21, 2023
Merged

Remove log4j dependencies #367

merged 5 commits into from
Feb 21, 2023

Conversation

lbulej
Copy link
Member

@lbulej lbulej commented Oct 5, 2022

Attempt to completely remove log4j from apache-spark dependencies. Complements #365 which updated slf4j-log4j12 to pull in reload4j, but did not remove direct the Spark dependency on log4j. With log4j removed, there seem to be some rough edges (missing appenders), probably configuration related. If this can be sorted out, we can do a minimal-change maintenance release, otherwise we'll see if the problems go away on its own after updating most of the dependencies.

This allows keeping the core dependencies separate from the overrides.
On the other hand, the overrides must target specific packages, which
in case of some benchmarks requires listing specific Netty components,
because we cannot simply override the version of the netty-all package.
This is to avoid pulling in the log4j-1.2.17.jar. The functionality
should be replaced by reload4j, which gets pulled in by slf4j-log4j12
starting with version 1.7.36. Because reload4j did not find Spark's
default logging configuration file (log4j.properties), it was placed in
the apache-spark benchmark resource directory so that reload4j can find
it. This appears to provide a reasonable logging configuration again.

The commons-logging package should be subsumed by the jcl-over-slf4j
logging bridge.
@lbulej lbulej force-pushed the topic/remove-log4j branch from f21b0d3 to e2fc973 Compare February 8, 2023 17:07
@lbulej
Copy link
Member Author

lbulej commented Feb 8, 2023

I have moved Spark's log4j configuration file to a place where reload4j can find it, so we should be back to normal logging configuration.

@lbulej lbulej marked this pull request as ready for review February 8, 2023 17:15
@lbulej lbulej changed the title Explicitly remove log4j from apache-spark Remove log4j dependencies Feb 20, 2023
@lbulej
Copy link
Member Author

lbulej commented Feb 20, 2023

With a few minor updates, the suite seems to be free of log4j and does not print logging noise.

Overall, logging seems to be a somewhat messy issue, especially with libraries using different logging solutions. We should be able to get somewhat "unified" control over dependencies that use org.slf4j, but there are still some that use java.util.logging directly (which cannot be configured by just placing a properties file on the class path). I'll revisit this after updating major libraries, but first I would like to get a log4j-free maintenance release (with updated JNA) out.

@lbulej lbulej requested a review from farquet February 20, 2023 09:25
Copy link
Collaborator

@farquet farquet left a comment

Choose a reason for hiding this comment

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

LGTM

@lbulej lbulej merged commit 19d00d9 into master Feb 21, 2023
@lbulej lbulej deleted the topic/remove-log4j branch February 27, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants