spark
spark copied to clipboard
[SPARK-40511][BUILD][CORE] Upgrade slf4j to 2.0.2
What changes were proposed in this pull request?
This pr aims to upgrade slf4j from 1.7.36 to 2.0.x, the main change as follows:
- Upgrade slf4j version from 1.7.36 to 2.0.x
- Use
log4j-slf4j2-impl
instead oflog4j-slf4j-impl
refer to https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/index.html - Adapt to the implementation of
Logging.isLog4j2()
, slf4j2 usesSLF4JServiceProvider
instead ofStaticLoggerBinder
Why are the changes needed?
slf4j 1.7.36 is no longer actively developed, and the current stable and actively developed version of slf4j is 2.0.x.
- https://www.slf4j.org/news.html
- https://www.slf4j.org/faq.html#changesInVersion200
- https://www.slf4j.org/download.html
Does this PR introduce any user-facing change?
No
How was this patch tested?
Pass GitHub Actions
Test API compatibility first
close and wait log4j 2.19.x
@LuciferYang https://search.maven.org/artifact/org.apache.logging.log4j/log4j-slf4j2-impl has been released!
Thanks @martin-g
reopen this due to log4j 2.19.0 released and Spark already upgrade to use this version, test first
friendly ping @viirya @HyukjinKwon @dongjoon-hyun @srowen @martin-g
This pr try to upgrade slf4j from 1.x to 2.x, but I don't think it is an appropriate time to upgrade it now due to slf4j 2.0.x still released frequently, so I will keep this pr as draft for a period of time and continuous testing it.
so I will keep this pr as draft for a period of time and continuous testing it.
So this is changed?
so I will keep this pr as draft for a period of time and continuous testing it.
So this is changed?
may be a mistake, re-change to draft
mvn test Java8 + hadoop-2 profile
with this pr , all test passed
2.0.3 released, let us wait more time
mvn test Java17 + hadoop-3
profile with this pr , all test passed
Is this ready to merge?
I think the code is ready for review,but I'm not sure whether the slf4j will continue to release the versions frequently recently, I set this one to ready for review
first.
should we merge this one first ?
Yep. Thank you, @LuciferYang and all. Merged to master.
Thanks @martin-g @dongjoon-hyun @HyukjinKwon @srowen @viirya @itholic