[ZOOKEEPER-4820] Fix propagation of Logback dependencies

[ppkarwasz]

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 available on Zookeeper's site.

[shoothzj]

maybe we can mark this dependency as runtime optional instead of test?

[ppkarwasz]

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.

[ctubbsii]

I have a preference for optional runtime, but this works too.

[ctubbsii]

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 the meaning of these. 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).

[ctubbsii]

I see it was changed while I was replying to the other comment. I like this better for the reasons I've stated (I think it communicates intent better), but I do concede that marking it as test scope could have also worked.

[ppkarwasz]

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 the meaning of these. 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 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]

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.

[ctubbsii]

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 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 review or an opinion there.

[ctubbsii]

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.

[ctubbsii]

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.

[eolivelli]

with this change are you able to see the logs while running the unit tests ?