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

[SPARK-47194][BUILD] Upgrade log4j to 2.23.0 #45292

Closed
wants to merge 1 commit into from

Conversation

LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

This pr aims to upgrade log4j2 from 2.22.1 to 2.23.0.

Why are the changes needed?

The new version contains some bug fixes:

The full release notes as follows:

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the BUILD label Feb 27, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending CIs).

Thank you, @LuciferYang !

@LuciferYang
Copy link
Contributor Author

k8s-it has failed, let me take a look first

@LuciferYang LuciferYang marked this pull request as draft February 27, 2024 20:10
@LuciferYang
Copy link
Contributor Author

It seems that the -Dlog4j2.debug option may not be working in 2.23.0, perhaps we should skip this upgrade. I have tested the following scenarios:

  1. run dev/make-distribution.sh --tgz to build a Spark Client
  2. add log4j2.properties and spark-defaults.conf with the same content as test case Verify logging configuration is picked from the provided SPARK_CONF_DIR/log4j2.properties
log4j2.properties 

# This log4j config file is for integration test SparkConfPropagateSuite.
rootLogger.level = debug
rootLogger.appenderRef.stdout.ref = console

appender.console.type = Console
appender.console.name = console
appender.console.target = SYSTEM_ERR
appender.console.layout.type = PatternLayout
appender.console.layout.pattern = %d{HH:mm:ss.SSS} %p %c: %maxLen{%m}{512}%n%ex{8}%n
spark-defaults.conf

spark.driver.extraJavaOptions -Dlog4j2.debug
spark.executor.extraJavaOptions -Dlog4j2.debug
spark.kubernetes.executor.deleteOnTermination false
  1. run bin/run-example SparkPi

When using log4j 2.22.1, we can have the following log:

...

TRACE StatusLogger DefaultConfiguration cleaning Appenders from 1 LoggerConfigs.
DEBUG StatusLogger Stopped org.apache.logging.log4j.core.config.DefaultConfiguration@384ad17b OK
TRACE StatusLogger Reregistering MBeans after reconfigure. Selector=org.apache.logging.log4j.core.selector.ClassLoaderContextSelector@5852c06f
TRACE StatusLogger Reregistering context (1/1): '5ffd2b27' org.apache.logging.log4j.core.LoggerContext@31190526
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=StatusLogger'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=ContextSelector'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=Loggers,name=*'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=Appenders,name=*'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=AsyncAppenders,name=*'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=AsyncLoggerRingBuffer'
TRACE StatusLogger Unregistering but no MBeans found matching 'org.apache.logging.log4j2:type=5ffd2b27,component=Loggers,name=*,subtype=RingBuffer'
DEBUG StatusLogger Registering MBean org.apache.logging.log4j2:type=5ffd2b27
DEBUG StatusLogger Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=StatusLogger
DEBUG StatusLogger Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=ContextSelector
DEBUG StatusLogger Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=Loggers,name=
DEBUG StatusLogger Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=Appenders,name=console
TRACE StatusLogger Using default SystemClock for timestamps.
DEBUG StatusLogger org.apache.logging.log4j.core.util.SystemClock supports precise timestamps.
TRACE StatusLogger Using DummyNanoClock for nanosecond timestamps.
DEBUG StatusLogger Reconfiguration complete for context[name=5ffd2b27] at URI /Users/yangjie01/Tools/4.0/spark-4.0.0-SNAPSHOT-bin-3.3.6/conf/log4j2.properties (org.apache.logging.log4j.core.LoggerContext@31190526) with optional ClassLoader: null
DEBUG StatusLogger Shutdown hook enabled. Registering a new one.
...

But when using log4j 2.23.0, no logs related to StatusLogger are printed.

cc @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

Thank you for the investigation. +1 for skipping.

@LuciferYang
Copy link
Contributor Author

OK, Let me close this pr first. Thanks @dongjoon-hyun

@panbingkun
Copy link
Contributor

panbingkun commented Feb 29, 2024

@LuciferYang @dongjoon-hyun
I added configuration status = debug to the file log-config-test-log4j.properties and run bin/run-example SparkPi locally. It seems that the expected log has been printed out. The supplementary testing for this PR is here: #45326. Let's run it through GA first.

image image

Ref:
https://logging.apache.org/log4j/log4j-2.2/manual/configuration.html
image

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Feb 29, 2024

@LuciferYang @dongjoon-hyun I added configuration status = debug to the file log-config-test-log4j.properties and run bin/run-example SparkPi locally. It seems that the expected log has been printed out. The supplementary testing for this PR is here: #45326. Let's run it through GA first.

image image
Ref: https://logging.apache.org/log4j/log4j-2.2/manual/configuration.html image

Is this an expected behavior change? I didn't see any related content in the release notes for 2.23.0.

@panbingkun
Copy link
Contributor

After testing, the problematic UT mentioned above has passed,
https://github.com/panbingkun/spark/actions/runs/8089725938/job/22106109827
image

@dongjoon-hyun
Copy link
Member

Thank you for checking, @panbingkun . Ya, it looks like a breaking change to me

@dongjoon-hyun
Copy link
Member

Shall we report back to the upstream community?

@LuciferYang
Copy link
Contributor Author

Shall we report back to the upstream community?

+1

@panbingkun
Copy link
Contributor

Let me try to submit an issue to the log4j community.

@LuciferYang
Copy link
Contributor Author

Let me try to submit an issue to the log4j community.

Thanks @panbingkun

@panbingkun
Copy link
Contributor

Let me try to submit an issue to the log4j community.

An issue has been submitted to the log4j2 community:
apache/logging-log4j2#2337

The root cause is recorded here:
#45326 (comment)

@dongjoon-hyun
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants