ctubbsii commented on code in PR #2293:
URL: https://github.com/apache/zookeeper/pull/2293#discussion_r2274943771
##########
zookeeper-server/src/test/java/org/apache/zookeeper/test/LoggerTestTool.java:
##########
Review Comment:
For this and the tests that use it, I think it would be fine to leave
`logback` in as a test dependency `<scope>test</scope>`.
It is ill-advised to do the alternative, which requires modifying the global
output stream, as that can affect other threads in pretty substantial ways,
like Maven or maven-surefire-plugin threads, during the build.
##########
zookeeper-contrib/zookeeper-contrib-zooinspector/pom.xml:
##########
@@ -90,8 +90,8 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
Review Comment:
```suggestion
<artifactId>slf4j-simple</artifactId>
<scope>test</scope>
```
##########
zookeeper-contrib/zookeeper-contrib-zooinspector/src/test/java/org/apache/zookeeper/inspector/LoggerTest.java:
##########
@@ -31,12 +31,12 @@ public class LoggerTest {
String testMessage = "a test message";
@Test
- public void testLogStdOutConfig() {
+ public void testLogStdErrConfig() {
ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream();
- PrintStream realStdOut = System.out;
- System.setOut(new PrintStream(byteArrayOutputStream));
+ PrintStream realStdErr = System.err;
+ System.setErr(new PrintStream(byteArrayOutputStream));
LOG.info(testMessage);
- System.setOut(realStdOut);
+ System.setErr(realStdErr);
Review Comment:
This test isn't particularly useful as part of ZooKeeper, and could just be
deleted. This isn't even an integration test, and doesn't use any ZK code...
it's just literally testing the logging dependency API, which can be assumed to
work from upstream testing (or at least, if it broke upstream, it would be
noticed long before this test runs). Plus, it's not generally a good idea to
modify the global system output streams.
##########
zookeeper-contrib/zookeeper-contrib-loggraph/pom.xml:
##########
@@ -53,8 +53,8 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
Review Comment:
This can be set to `<scope>test</scope>`. It's really only needed as a
runtime dependency for modules that do assembly (such as the fatjar module, the
zookeeper-assembly module, and maybe the zooinspector module, which I see also
has some kind of assembly). It should never be needed as a compile time
dependency.
```suggestion
<artifactId>slf4j-simple</artifactId>
<scope>test</scope>
```
##########
zookeeper-server/pom.xml:
##########
@@ -75,12 +75,8 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
- </dependency>
- <dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-classic</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
Review Comment:
```suggestion
<artifactId>slf4j-simple</artifactId>
<scope>test</scope>
```
`runtime` may also be an appropriate scope here, to document to users that
depend on the `zookeeper` artifactId that this, or an alternative
implementation is needed at runtime. If this is marked as a runtime dependency
rather than a test dependency, I would also add `<optional>true</optional>` so
that it is not automatically picked up by other projects that depend on the
zookeeper-server jar during their Maven builds, for their slf4j runtime
implementation, as a transitive dependency. Otherwise, everybody would have to
add `<exclusions>` elements to their `<dependency>` on `zookeeper-server`, the
same way ZooKeeper has to for the `kerby-config` dependency, which would
otherwise bring in the `slf4j-log4j12` runtime. This should have been done with
logback as well, but never was. This isn't necessary if it's set to test scope,
since test scopes are never added automatically as a transitive dependency.
##########
zookeeper-contrib/zookeeper-contrib-rest/pom.xml:
##########
@@ -69,8 +69,8 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
Review Comment:
```suggestion
<artifactId>slf4j-simple</artifactId>
<scope>test</scope>
```
##########
zookeeper-contrib/zookeeper-contrib-fatjar/pom.xml:
##########
@@ -91,8 +91,8 @@
<artifactId>snappy-java</artifactId>
</dependency>
<dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-core</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
Review Comment:
```suggestion
<artifactId>slf4j-simple</artifactId>
<scope>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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
