-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Do not merge] Update slf4j version #33574
base: master
Are you sure you want to change the base?
Conversation
The class path of slf4j has been changed from org/slf4j/impl to org/slf4j/jul. Two failed tests: - :runners:google-cloud-dataflow-java:worker:validateShadedJarContainsSlf4jJdk14 - :runners:google-cloud-dataflow-java:worker:validateShadedJarDoesntLeakNonProjectClasses
The failed tests are under org.apache.beam.runners.dataflow.worker.HotKeyLoggerTest
The previous version has a compile dependency on slf4j 1.x binding, which would no longer work with slf4j 2.x.
triggered a performance test: https://github.com/apache/beam/actions/runs/13145020125 |
r: @Abacn |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Had a few comments.
when it is ready to merge good to announce it in CHANGES.md
@@ -250,6 +250,14 @@ project.task('validateShadedJarContainsSlf4jJdk14', dependsOn: 'shadowJar') { | |||
} | |||
} | |||
|
|||
project.tasks.withType(Test).configureEach { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are they needed? tests currently using Java11 should be fine as https://openjdk.org/jeps/403 is enforced in Java16+ only.
Did you run tests on Java 17+ locally? If so did the test experience similar issue on master branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took them out and triggered the tests again on github. Will see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the test is fine. However, still good to confirm
Did you run tests on Java 17+ locally? If so did the test experience similar issue on master branch?
@@ -43,8 +43,8 @@ dependencies { | |||
implementation library.java.commons_lang3 | |||
implementation library.java.joda_time | |||
implementation library.java.slf4j_api | |||
implementation library.java.spark_streaming | |||
implementation library.java.spark_core | |||
implementation library.java.spark3_streaming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move sparkreceiver/2 to sparkreceiver/3 or simply sparkreceiver/. The package name is unchanged so that release is unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sent an email to dev list regarding this change. I will wait a couple days to see if there is any objection or other suggestion.
Run Java PreCommit |
Run Java_Examples_Dataflow_Java21 PreCommit |
Java PreCommit testOnNewWorkerMetadata_redistributesBudget (org.apache.beam.runners.dataflow.worker.streaming.harness.FanOutStreamingEngineWorkerHarnessTest) failed due to #28957 not related to this change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Upgrade slf4j from 1.7.30 to 2.0.16