[improve][dependency] Remove jul-to-slf4j #16320
Conversation
So that we can work with JDK17; otherwise, it will fail with "Unsupported class file major version 61". Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
This reverts commit 614f5bc.
| @@ -558,7 +558,6 @@ BSD 2-Clause License | |||
| MIT License | |||
| * Java SemVer -- com.github.zafarkhaja-java-semver-0.9.0.jar -- licenses/LICENSE-SemVer.txt | |||
| * SLF4J -- licenses/LICENSE-SLF4J.txt | |||
| - org.slf4j-jul-to-slf4j-1.7.32.jar | |||
| - org.slf4j-slf4j-api-1.7.32.jar | |||
| - org.slf4j-jcl-over-slf4j-1.7.32.jar | |||
There was a problem hiding this comment.
We need to redirect jcl-over-slf4j due to #1740 that it's an internal dependency to commons-logging and we should handle within Pulsar itself.
codelipenghui
added
release/important-notice
|
@codelipenghui thanks for your review! @eolivelli @merlimat could you give a look at this patch so that we can have an opportunity to include it in 2.11.0 as previously proposed in the mailing list? |
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Pulsar Bot Command match exactly without whitespace, perhaps we should trim before match comments.. |
|
|
/pulsarbot run-failure-checks |
|
CPP Tests ever passed at https://github.com/apache/pulsar/runs/7192710852?check_suite_focus=true, all later commits are merge commit. Broker Tests Group 1 ever passed at https://github.com/apache/pulsar/runs/7153335951?check_suite_focus=true, all later commits are merge commit. |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
It seems Broker Group 1 suite often timeout. |
|
/pulsarbot run-failure-checks |
|
Cool. I have been waiting for this for a loooooong time. |
|
Merge master tomorrow to retry running tests. Stop continue rerunning failure tests >_< |
|
OWASP failed on false positive cases, see also jetty/jetty.project#8161 (comment). I think it should be resolve on upstream soon later. |
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
"CI - Unit - Brokers - Broker Group 1" failed over days and the failures seem quick weird; for example, @codelipenghui @eolivelli it seems this patch block by those flaky tests. Could you help to confirm and if it's the case, advice how to make progress? |
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
|
@codelipenghui @eolivelli all required CI tasks passed. Could you please help on moving forward this patch? cc @merlimat |
### Motivation see also https://lists.apache.org/thread/65skwo492w2nfjwhb3d9y51roq13h8bs PulsarAdminImpl has a static block requires classpath contains exact either of: * slf4j-jdk14 * jul-to-slf4j It causes an issue that if user depends on neither slf4j-jdk14 nor jul-to-slf4j, PulsarAdminImpl will panic with NoClassDefFoundError. And thus, user should add one of them (basically jul-to-slf4j) even if they don't depend on it effectively. ### Modifications Remove jul-to-slf4j from all modules under Pulsar. The Pulsar project uses slf4j as the logging facade consistently. If a user want to add a dependency using a different logging framework, they should take care of the packaging strategy themselves. pulsar-sql has a dependency to slf4j-jdk14 which redirect slf4j to jul. Since this is a unidirectional redirection, it won't cause runtime error, but pulsar-sql will logging with jul framework, which is the same as with previous workaround.
I'm unsure whether I should create a proposal or start a discussion about this removal on dev@.
Motivation
see also https://lists.apache.org/thread/65skwo492w2nfjwhb3d9y51roq13h8bs
PulsarAdminImpl has a static block requires classpath contains exact either of:
It causes an issue that if user depends on neither slf4j-jdk14 nor jul-to-slf4j, PulsarAdminImpl will panic with NoClassDefFoundError. And thus, user should add one of them (basically jul-to-slf4j) even if they don't depend on it effectively.
Modifications
Remove jul-to-slf4j from all modules under Pulsar.
The Pulsar project uses slf4j as the logging facade consistently. If a user want to add a dependency using a different logging framework, they should take care of the packaging strategy themselves.
pulsar-sql has a dependency to slf4j-jdk14 which redirect slf4j to jul. Since this is a unidirectional redirection, it won't cause runtime error, but pulsar-sql will logging with jul framework, which is the same as with previous workaround.
cc @merlimat as the original author of this stuff.
cc @syhily who points me out this issue.
cc @codelipenghui @eolivelli
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesThe motivation and affected scope are described above.
Documentation
doc-not-needed