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-implinstead oflog4j-slf4j-implrefer to https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/index.html - Adapt to the implementation of
Logging.isLog4j2(), slf4j2 usesSLF4JServiceProviderinstead 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