You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2024/04/09 21:05:22 UTC
[PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
ppkarwasz opened a new pull request, #2155:
URL: https://github.com/apache/zookeeper/pull/2155
Java libraries should not have `runtime` dependencies on logging backends, since they are often included in third-party applications that might make a different choice of backend.
This PR:
* removes the Logback dependency from the runtime scope of the `zookeeper` artifact and moves it into the `test` Maven scope,
* adds Logback to `zookeeper-assembly`, which is used to produce the standalone [binary distribution](https://zookeeper.apache.org/releases) available on Zookeeper's site.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2359943237
@anmolnar,
I don't have enough permissions on JIRA to assign ZOOKEEPER issues. :wink:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2050722823
Sorry, one more comment: looking at this closely, it looks like some updates might be needed to fix the logging dependencies in the contrib poms, too. They might also need similar changes as `zookeeper-server/pom.xml`, but I'm not very familiar with them... one of them, the fatjar one, based on its name, looks like it might need changes similar to the assembly... but I'm not really sure what purpose that contrib project serves, so I can't be certain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2048289428
> > The meaning is pretty much up to each developer to interpret. I interpret `provided` as "we don't bundle Logback, but you **need** it for Zookeeper to run". Regarding `optional` I interpret it as "some Zookeeper features only work with Logback" (a Logback appender maybe?).
>
> Maven [actually defines](https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html) the [meaning of these](https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html). I think your interpretation is pretty much correct, because it aligns with these definitions, but I don't think it's as subjective as you suggest. These definitions are what developers use to implement behavioral differences.
Inside the Zookeeper project I agree: there are behavioral differences. But if Zookeeper is used as a dependency in another project, the differences fade.
>> I think the message that should be sent to users is "Zookeeper prefers/recommends Logback, but it will work with whatever SLF4J implementation your provide him".
>
> I would say "happens to use" rather than "prefers/recommends", but yes, I think that message is basically the right message. But I think that message can be communicated with optional, and a basic understanding of Java logging frameworks.
Yes, it makes sense. I might steal your interpretation and add an optional runtime dependency on Logback to `log4j-to-slf4j`, although the proposal to make Log4j Core an optional runtime dependency of `log4j-slf4j2-impl` in [LOG4J2-3601](https://issues.apache.org/jira/browse/LOG4J2-3601) was not met with a big applause. :stuck_out_tongue_winking_eye: [Disclaimer: I understand that these examples give the user only one serious choice of logging backend, while Zookeeper has two]
> > Inside the Zookeeper project their usefulness is also somehow limited: the Maven Compiler and Surefire plugins will use them, but the Maven Assembly plugin will not package them in the `tar.gz` archive.
> > So the only difference between runtime `optional` and `test` is that Logback is available at compilation time. This **is not** an advantage: if Zookeeper fails to compile without Logback, it means that it references some Logback-specific classes.
>
> This is not true. There's actually no difference here. If it's marked optional alone, but still in the default "compile" scope, it will be available at compilation time. However, if it's marked "runtime", it is not available at compilation time... only test. That's why "optional" is frequently used in conjunction with "runtime" scope. So, there's actually no difference in behavior between these... it really just comes down to communicating intent.
Sorry, I was thinking `compile optional`. Thanks for correcting me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2050719455
I did notice that there's a failing test, `zookeeper-contrib/zookeeper-contrib-zooinspector/src/test/java/org/apache/zookeeper/inspector/LoggerTest.java`
I'm not sure if the test was failing before this. But, it doesn't look like a good test. It looks like it's testing the logging framework itself... which is outside the scope of ZK. That's a kind of test I would expect from upstream in the logging library. It makes no sense to be included in the zookeeper inspector contrib tests, since it has nothing to do with ZK or the inspector contrib. I would recommend just deleting the test. It's not needed, and serves no purpose for ZK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2361310720
> @anmolnar,
>
> I don't have enough permissions on JIRA to assign ZOOKEEPER issues. 😉
What's your jira id?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2362016552
> > > @anmolnar,
> > > I don't have enough permissions on JIRA to assign ZOOKEEPER issues. 😉
> >
> >
> > What's your jira id?
>
> `pkarwasz` with one `p`.
It's done. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2048181083
> Personally I am not a big fan of `optional` or `provided` dependencies: their role is documentation only.
It affects behavior, so I would not say it's documentation only. But even if it were, there is value in documentation... in communicating intent to downstream consumers of the project.
> The meaning is pretty much up to each developer to interpret. I interpret `provided` as "we don't bundle Logback, but you **need** it for Zookeeper to run". Regarding `optional` I interpret it as "some Zookeeper features only work with Logback" (a Logback appender maybe?).
Maven [actually defines](https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html) the [meaning of these](https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html). I think your interpretation is pretty much correct, because it aligns with these definitions, but I don't think it's as subjective as you suggest. These definitions are what developers use to implement behavioral differences.
Optional just means you have the option to exclude it and that Maven does so by default. It doesn't say anything about what behavioral changes might result or what you might require in its place, though, if you choose to exclude it. For that, you kind of have to know something about what is being excluded and why. A little understanding of Java logging frameworks like slf4j close this gap, though, and it will be clear to most users that the runtime implementation of slf4j is interchangeable, and that's why this implementation is optional. So, I don't think it'd be a problem to mark it as optional.
> I think the message that should be sent to users is "Zookeeper prefers/recommends Logback, but it will work with whatever SLF4J implementation your provide him".
I would say "happens to use" rather than "prefers/recommends", but yes, I think that message is basically the right message. But I think that message can be communicated with optional, and a basic understanding of Java logging frameworks.
> Inside the Zookeeper project their usefulness is also somehow limited: the Maven Compiler and Surefire plugins will use them, but the Maven Assembly plugin will not package them in the `tar.gz` archive.
>
> So the only difference between runtime `optional` and `test` is that Logback is available at compilation time. This **is not** an advantage: if Zookeeper fails to compile without Logback, it means that it references some Logback-specific classes.
This is not true. There's actually no difference here. If it's marked optional alone, but still in the default "compile" scope, it will be available at compilation time. However, if it's marked "runtime", it is not available at compilation time... only test. That's why "optional" is frequently used in conjunction with "runtime" scope. So, there's actually no difference in behavior between these... it really just comes down to communicating intent.
>
> **Remark**: The way Logback is treated in this PR is similar to the Jetty dependency. Jetty is included in the `tar.gz`, but there are no runtime dependencies on it.
This is a strange comparison, because jetty is actually marked as provided, which puts it on the compile and runtime classpaths. So, contrary to what you just said, there actually *is* a runtime dependency for it. I would actually argue that these should just be regular compile (or runtime) dependencies, though, and not marked provided, since it's probably not expected that jetty will be provided by another project, since it's provided by this one. But that's outside the scope of this change.
In any case, because ZooKeeper does actually have a compile time dependency on logback for its tests, I think marking it as a test dependency is fine, since it achieves the same thing as a runtime optional dependency, but I would still prefer runtime optional over test, because it communicates that something is being omitted from the runtime scope that may need to be added in order to achieve particular functionality (logging).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#discussion_r1559857699
##########
zookeeper-server/pom.xml:
##########
@@ -177,6 +169,11 @@
<artifactId>tools</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
There are two requirements here:
1. Ensure it's available for the tests, which require it, and
2. Ensure it is excluded by default from any project that depends on this one.
Specifying it as a test scope achieves both of these. And, because runtime scope is included in the test classpath, specifying it as an optional runtime dependency would also achieve both of these.
However, specifying an optional runtime dependency here would be a bit more helpful, because users generally familiar with Java logging frameworks would understand that there's a runtime component that they need to add themselves. This is not clear when it's only added to the test scope.
##########
zookeeper-assembly/pom.xml:
##########
@@ -103,6 +103,10 @@
<groupId>jline</groupId>
<artifactId>jline</artifactId>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ </dependency>
Review Comment:
This is good because it actually is an explicit dependency of the assembly. This project requires it in the bundle it creates. So, regardless of what happens in the other pom.xml, this is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2310336972
> with this change are you able to see the logs while running the unit tests ?
Yes. I confirmed this. cc @eolivelli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2047451913
> maybe we can mark this dependency as runtime optional instead of test?
Personally I am not a big fan of `optional` or `provided` dependencies: their role is documentation only. The meaning is pretty much up to each developer to interpret. I interpret `provided` as "we don't bundle Logback, but you **need** it for Zookeeper to run". Regarding `optional` I interpret it as "some Zookeeper features only work with Logback" (a Logback appender maybe?). I think the message that should be sent to users is "Zookeeper prefers/recommends Logback, but it will work with whatever SLF4J implementation your provide him".
Inside the Zookeeper project their usefulness is also somehow limited: the Maven Compiler and Surefire plugins will use them, but the Maven Assembly plugin will not package them in the `tar.gz` archive.
So the only difference between runtime `optional` and `test` is that Logback is available at compilation time. This **is not** an advantage: if Zookeeper fails to compile without Logback, it means that it references some Logback-specific classes.
**Remark**: The way Logback is treated in this PR is similar to the Jetty dependency. Jetty is included in the `tar.gz`, but there are no runtime dependencies on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar merged PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2359699808
This looks like it was done and reviewed favorably, but never actually merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#discussion_r1559874907
##########
zookeeper-server/pom.xml:
##########
@@ -177,6 +169,11 @@
<artifactId>tools</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
I don't really have a preference, since functionally the two declarations are the same.
I switched to `optional` in the `runtime` scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#discussion_r1559893434
##########
zookeeper-assembly/pom.xml:
##########
@@ -103,6 +103,10 @@
<groupId>jline</groupId>
<artifactId>jline</artifactId>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ </dependency>
Review Comment:
By convention, Apache Accumulo marks all of its assembly dependencies as "optional", just in case somebody actually depends on the tarball out of a Maven repository, it won't resolve all the dependencies that were used to build the tarball. We could have used "provided" instead for that purpose. But ultimately, it doesn't matter if you're not publishing the assembly to Maven. The conventions for dealing with assemblies in Maven repos kind of break all the norms used for jars. Either way, everything bundled in the assembly should be explicitly listed as a dependency, regardless of whatever other scopes/modifiers you put on them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2046946911
maybe we can mark this dependency as runtime optional instead of test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2050713619
> Yes, it makes sense. I might steal your interpretation and add an optional runtime dependency on Logback to `log4j-to-slf4j`, although the proposal to make Log4j Core an optional runtime dependency of `log4j-slf4j2-impl` in [LOG4J2-3601](https://issues.apache.org/jira/browse/LOG4J2-3601) was not met with a big applause. 😜 [Disclaimer: I understand that these examples give the user only one serious choice of logging backend, while Zookeeper has two]
LOG4J2-3601 was a problem because:
1. it was an unexpected change in behavior from `log4j-slf4j-impl` to `log4j-slf4j2-impl` to be more "flexible" that nobody asked for and made things inconvenient, and
2. the new use cases it supported were nonsensical (they added an extra layer of log4j that wasn't needed and adding it for those use cases would have been bad practice)
I suspect adding logback as an optional runtime dependency to `log4j-to-slf4j` is probably not a great idea, but for different reasons. For starters, choosing logback seems arbitrary and a less good default than, say, `slf4j-simple`. slf4j has many runtimes that are all optional, and logback is only one of them. It's not really necessary to list all of them as optional runtimes, or any of them. The website does a better job documenting how to bind runtimes than the XML in a POM would. It's also a different situation than here with ZK, where logback is in a different position because ZK chose logback as its default (and what it ships with). But, ZK also needs to more easily support other runtimes when ZK is used like a library dependency. That's very different than `log4j-to-slf4j` or `slfj-api`, where there is no default runtime selected. In any case, that's a discussion for the log4j community. Feel free to tag me in any discussions of that, though, if you want me to provide a re
view or an opinion there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2361561061
> > @anmolnar,
> > I don't have enough permissions on JIRA to assign ZOOKEEPER issues. 😉
>
> What's your jira id?
`pkarwasz` with one `p`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZOOKEEPER-4820] Fix propagation of Logback dependencies [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#issuecomment-2359886678
@ctubbsii Cool, thanks for the heads-up. I'm happy to commit patches for the upcoming 3.9.3 release.
@ppkarwasz Thanks for the contribution, would you please assign the Jira to yourself?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org