You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/01/05 15:12:20 UTC
[GitHub] [zookeeper] anmolnar opened a new pull request #1793: ZOOKEEPER-4427 Migrate to Logback
anmolnar opened a new pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793
This is the first commit of Logback migration task.
1. Replaced log4j dependencies with logback alternatives,
2. Replaced log4j.properties files with logback.xml
3. Refactored unit tests to use logback for log capturing.
This patch doesn't cover the zookeeper-contrib projects, only the main codebase.
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789593432
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -504,22 +504,20 @@ The ZooKeeper service can be monitored in one of three primary ways:
### Logging
ZooKeeper uses **[SLF4J](http://www.slf4j.org)**
-version 1.7.5 as its logging infrastructure. For backward compatibility it is bound to
-**LOG4J** but you can use
-**[LOGBack](http://logback.qos.ch/)**
-or any other supported logging framework of your choice.
-
-The ZooKeeper default *log4j.properties*
-file resides in the *conf* directory. Log4j requires that
-*log4j.properties* either be in the working directory
+version 1.7.5 as its logging infrastructure. By default ZooKeeper is shipped with
Review comment:
I'd recommend being less tightly coupled to the current patch version in this document. Just "1.7" should suffice here.
--
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
[GitHub] [zookeeper] cnauroth commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
cnauroth commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r779122862
##########
File path: pom.xml
##########
@@ -459,6 +459,7 @@
<!-- dependency versions -->
<slf4j.version>1.7.30</slf4j.version>
+ <logback-version>1.2.7</logback-version>
Review comment:
1.2.10 is the latest non-alpha release. Do you want to upgrade to that?
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/LoggerTestTool.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.Layout;
+import ch.qos.logback.core.OutputStreamAppender;
+import ch.qos.logback.core.encoder.LayoutWrappingEncoder;
+import org.slf4j.LoggerFactory;
+
+import java.io.ByteArrayOutputStream;
+
+public class LoggerTestTool<T> implements AutoCloseable {
Review comment:
+1, this is nice!
Do you think instead of parameterizing the type, it would make sense for the constructor to just accept `Class<?>` directly? It seems like all of the call sites just end up instantiating it as `LoggerTestTool<?>`, so they aren't getting much value from capturing the specific class type. The underlying SLF4J interface also operates on `Class<?>`.
##########
File path: zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/resources/logback.xml
##########
@@ -0,0 +1,112 @@
+<!--
+ Copyright 2022 The Apache Software Foundation
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ Define some default values that can be overridden by system properties
+-->
+<configuration>
Review comment:
Optional: Maybe this file can be simplified by removing unused properties and commented-out lines, because the tests just end up using console anyway.
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java
##########
@@ -48,33 +37,37 @@
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
+import org.apache.zookeeper.test.LoggerTestTool;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.LineNumberReader;
+import java.io.StringReader;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.List;
public class Log4jAuditLoggerTest extends QuorumPeerTestBase {
Review comment:
Shall we rename this class to remove mention of Log4J?
##########
File path: zookeeper-server/src/test/resources/logback.xml
##########
@@ -0,0 +1,112 @@
+<!--
+ Copyright 2022 The Apache Software Foundation
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ Define some default values that can be overridden by system properties
+-->
+<configuration>
Review comment:
Same comment here about potentially simplifying test logging configuration files.
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1022993084
Committed to master branch.
I restarted CI many times, the last failure is unrelated to the patch.
--
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
[GitHub] [zookeeper] c3ivodujmovic commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
c3ivodujmovic commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1032804678
@eolivelli @anmolnar did this merge?
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1032805865
@c3ivodujmovic yes
We are VOTING on ZooKeeper 3.8.0 release candidate 0, please participate in the VOTE
--
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r779468246
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/LoggerTestTool.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.Layout;
+import ch.qos.logback.core.OutputStreamAppender;
+import ch.qos.logback.core.encoder.LayoutWrappingEncoder;
+import org.slf4j.LoggerFactory;
+
+import java.io.ByteArrayOutputStream;
+
+public class LoggerTestTool<T> implements AutoCloseable {
Review comment:
Good point. I removed the type parameter entirely.
--
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r792462014
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -504,22 +504,20 @@ The ZooKeeper service can be monitored in one of three primary ways:
### Logging
ZooKeeper uses **[SLF4J](http://www.slf4j.org)**
-version 1.7.5 as its logging infrastructure. For backward compatibility it is bound to
-**LOG4J** but you can use
-**[LOGBack](http://logback.qos.ch/)**
-or any other supported logging framework of your choice.
-
-The ZooKeeper default *log4j.properties*
-file resides in the *conf* directory. Log4j requires that
-*log4j.properties* either be in the working directory
+version 1.7.5 as its logging infrastructure. By default ZooKeeper is shipped with
Review comment:
This is done.
--
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
[GitHub] [zookeeper] cnauroth commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
cnauroth commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r792089755
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
Thank you. That confirms what I saw.
> To be honest I don't plan making any changes in this build process.
That makes sense. If we need to address anything in this area for release, it can be handled as separate 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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r779443100
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java
##########
@@ -48,33 +37,37 @@
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
+import org.apache.zookeeper.test.LoggerTestTool;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.LineNumberReader;
+import java.io.StringReader;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.List;
public class Log4jAuditLoggerTest extends QuorumPeerTestBase {
Review comment:
I should rename the class as well, not just the test.
Actually it should be `Slf4jAuditLogger` to be precise.
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/audit/Log4jAuditLoggerTest.java
##########
@@ -48,33 +37,37 @@
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
+import org.apache.zookeeper.test.LoggerTestTool;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.LineNumberReader;
+import java.io.StringReader;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.List;
public class Log4jAuditLoggerTest extends QuorumPeerTestBase {
Review comment:
Yes, I should rename the class as well, not just the test.
Actually it should be `Slf4jAuditLogger` to be precise.
--
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r790670154
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
@cnauroth I compared the binary and source distribution in terms of lincensing and I think we don't have a separate build process for that. They contain the same LICENSE and NOTICE files. Both the binary and source distribution also contains the per-jar lincense.txt files, but the binary distribution includes the jar files themselves. To be honest I don't plan making any changes in this build process. I believe this patch is already aligned with the current habit of releasing.
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1005871292
@ctubbsii Thanks for the useful feedback, I really appreciate it. Isn't log4j2's config file also XML based? Afaik log4j1 was the only one which still uses properties file, but I'm not sure.
I also need some help with the contrib projects, because currently they don't build. The reason is that I removed log4j dependencies from the parent pom and can't figure out how to put them back for only the. contrib projects
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r778928263
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/LoggerTestTool.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.Layout;
+import ch.qos.logback.core.OutputStreamAppender;
+import ch.qos.logback.core.encoder.LayoutWrappingEncoder;
+import org.slf4j.LoggerFactory;
+
+import java.io.ByteArrayOutputStream;
+
+public class LoggerTestTool<T> implements AutoCloseable {
Review comment:
Putting all the log capture test code in one place was a really nice decision. The implementation is pretty simple, too. Not sure what the equivalent in log4j2 or another slf4j binding would look like. So, logback here makes sense, if the alternatives would be substantially worse.
An alternative would be to make better use of mocking in the tests that use this. Capturing the logs is not usually an ideal way to do unit testing. But, this is a nice simple replacement for existing tests.
##########
File path: pom.xml
##########
@@ -608,11 +602,13 @@
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<version>${junit.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${mockito.version}</version>
+ <scope>test</scope>
Review comment:
It's not a good idea to put the `<scope>` in the `<dependencyManagement>` section of the POM, because that will propagate, and require the `<scope>compile</scope>` to be explicitly added (normally, it's not needed, because it's the default scope). It's better to just manage the version in the `<dependencyManagement>` section, and defer adding a scope until the dependency is actually used.
It is unlikely that these dependencies are going to be used in a scope other than `test`, but the only reason to put the scope here is to omit it later when it is used. However, omitting it later will make it *appear* as though the scope is used is the `compile` scope, so it doesn't really have any benefit to adding it here.
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
These really should be runtime scoped dependencies, since they should not actually be used anywhere in the code at compile time.
--
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
[GitHub] [zookeeper] anmolnar edited a comment on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar edited a comment on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1016790773
Almost there @eolivelli hold your last approval for a minute. ;)
I've submitted docs changes recently and would like to take care of the licenses file based on our LEGAL ticket. Stay tuned.
Recipes and contrib projects will come in a follow-up patch.
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1016409755
@sourabhsparkala Yes, this patch will completely remove log4j from ZK. Unfortunately we don't have a release date yet.
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789101068
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
@anmolnar Yeah, it wasn't that way for log4j1, because log4j1 was used at compile time. However, part of this is to complete the migration to `slf4j-api` at compile time, so the runtime implementations can be chosen by users. So, no actual implementation jar should need to be `compile` scoped anymore. They can just be `runtime` (or `test` if it is needed to compile the tests; `test` implies `runtime` as well). The main point is that they aren't needed at `compile` time, because `slf4j-api` is used at compile time instead.
--
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
[GitHub] [zookeeper] ctubbsii commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1005894613
> @ctubbsii Thanks for the useful feedback, I really appreciate it. Isn't log4j2's config file also XML based? Afaik log4j1 was the only one which still uses properties file, but I'm not sure.
log4j1 supported XML and properties, but some options were only available with XML. With log4j2, everything can be done with either XML, JSON, properties, or even YAML. We use properties files with log4j2 for Accumulo (https://github.com/apache/accumulo/blob/main/assemble/conf/log4j2.properties)
--
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
[GitHub] [zookeeper] ctubbsii commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1033655149
> Is the vote open to anyone to participate? If so where can we vote?
Voting happens on the project mailing lists, whose addresses you can find on the project website, https://zookeeper.apache.org
Anybody can provide feedback on a release candidate, but at the Apache Software Foundation, only PMC members' votes count (are considered "binding") for a release.
--
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
[GitHub] [zookeeper] sourabhsparkala commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
sourabhsparkala commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1016352555
Hello All,
Requesting you to confirm if this is the fix to completely remove log4j from zookeeper? If yes, could you please provide a planned release date for this fix?
Thanks and Regards
Sourabh
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1015674774
The following commits have been pushed:
- Clean-up shell scripts under bin/.
- Remove Log4J CVEs from owaspSuppressions.xml
- Added `jmxConfigurator` to logback.xml as commented suggestion to register logback beans (not enabled by default)
Still outstanding:
- Update documentation
- Check zookeeper-recipes project
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1020019694
@cnauroth Please double check the PR and let me know what exactly is outstanding and required for your +1.
--
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
[GitHub] [zookeeper] symat commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788461125
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
I agree. @anmolnar is there a reason why these are not in 'test' 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
[GitHub] [zookeeper] symat commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788527162
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
Hmm... you are right. I didn't think this through. We still need to put logback to the binary distribution as we need some default log engine.
--
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
[GitHub] [zookeeper] cnauroth commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
cnauroth commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789072425
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
@ctubbsii , great point about the different needs for source vs. binary distribution. Thank you.
It's fine to externalize license files to the resources folder (which land in the lib folder for the binary distribution). However, I think there still needs to be some kind of pointer from top-level to those separate licenses. It seems like zookeeper-server/src/main/resources/NOTICE.txt is trying to serve that purpose. I don't see this file actually showing up though when I build a binary distribution. It only has the same LICENSE.txt and NOTICE.txt from the root. (Additionally, my reading of the licensing how-to is that this stuff should be handled in LICENSE instead of NOTICE.)
Can someone else double-check me by testing building a binary distribution? Maybe we have a separate build bug preventing inclusion of the right files in the binary distribution? If so, that could be a separate topic, but it should be addressed before our next release.
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1006442969
> @anmolnar , thanks for picking this up. It's looking good so far! I entered a few comments.
>
> Some additional areas of the codebase to consider, not currently in the pull request:
>
> * Shell scripts under bin/.
> * owaspSuppressions.xml can remove Log4J CVEs.
> * Minor: the native client has a comment mentioning Log4J in zk_log.c.
> * Various documentation under zookeeper-docs will need a rewrite to discuss Logback instead of Log4J.
> * Possibly some things in zookeeper-recipes reference Log4J?
> * zookeeper-server NOTICE.txt should be updated for Logback instead of Log4J. I'm not sure if the phrasing needs to take care to indicate the license terms. Logback is dual-licensed under LGPL and EPL. LGPL is a category X license, disallowed in Apache project distributions, so we need to take the dependency under EPL intsead.
> * zookeeper-server still has a few references to Log4J in the JMX functionality, mostly just comments and method naming, but good to clean up. (See `grep -ri log4j zookeeper-server` for the full list.)
>
> I know you already said that you were treating contrib as separate scope. I think it would be fair to split some of this other stuff out into separate tickets/pull requests too.
Thanks @cnauroth , that's a very nice list of outstanding items. Very useful, because it contains a lot of stuff that I didn't consider previously. First, I'd like to get the buy-in from the community, then I'll polish up as much as I can fit into this PR.
--
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
[GitHub] [zookeeper] ctubbsii commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1005897777
> I also need some help with the contrib projects, because currently they don't build. The reason is that I removed log4j dependencies from the parent pom and can't figure out how to put them back for only the. contrib projects
To fix the contribs, their POMs use the zookeeper-contrib parent POM, which itself points to the parent POM that you edited. You can leave the log4j stuff in the dependencyManagement section. That section only manages dependencies when they are used. Since they are still used in the contribs, you still need to manage them somewhere. Adding them back to the dependencyManagement section won't add them back as dependencies to the modules that moved to logback, so it's fine to have them 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
[GitHub] [zookeeper] anmolnar edited a comment on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar edited a comment on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1015674774
The following commits have been pushed:
- Clean-up shell scripts under bin/. (this is a new limitation: root logger is no longer configurable via system property)
- Remove Log4J CVEs from owaspSuppressions.xml
- Added `jmxConfigurator` to logback.xml as commented suggestion to register logback beans (not enabled by default)
Still outstanding:
- Update documentation
- Check zookeeper-recipes project
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788365338
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
You can put the entire text in there, but more importantly, you should not include it in here, because you haven't bundled Logback into the source. You should only include LICENSE/NOTICE information for things actually distributing as a bundle. This means the LICENSE/NOTICE files can be different between the source tarball (official release) and the distribution tarball (AKA "convenience binary") that actually includes the logback jar bundled.
So, these are the wrong files to modify. I'm not familiar enough with the ZK build to know how the convenience binaries are built, but there should be a separate mechanism to bundle LICENSE/NOTICE files into the convenience binary than these, which are for the source.
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1017235459
> The top-level pom.xml still contains some references to Log4J.
It's needed for contrib projects to build. I'll remove it in the next patch.
> zookeeperAdmin.md contains a java -cp ... example that references Log4J jars.
Thought it's only an example and the CP is probably already outdated, so not worth replacing it. I'll fix it anyway.
> ReconfigExceptionTest contains a comment with a java -cp ... command referencing Log4J jars.
I'll fix this too.
> ZooTrace contains a comment that "Log4J must be correctly configured".
Fixed.
> zookeeper-server/src/main/resources/NOTICE.txt still mentions Log4J (though I'm unclear on whether or not this file is actually getting into the distribution).
Weird. I've never come across this file, but replace it with logback alternatives.
--
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
[GitHub] [zookeeper] craiglawson commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
craiglawson commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789509507
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -504,22 +504,20 @@ The ZooKeeper service can be monitored in one of three primary ways:
### Logging
ZooKeeper uses **[SLF4J](http://www.slf4j.org)**
-version 1.7.5 as its logging infrastructure. For backward compatibility it is bound to
-**LOG4J** but you can use
-**[LOGBack](http://logback.qos.ch/)**
-or any other supported logging framework of your choice.
-
-The ZooKeeper default *log4j.properties*
-file resides in the *conf* directory. Log4j requires that
-*log4j.properties* either be in the working directory
+version 1.7.5 as its logging infrastructure. By default ZooKeeper is shipped with
Review comment:
Should this be version 1.7.30?
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789101068
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
@anmolnar Yeah, it wasn't that way for log4j1, because log4j1 was used at compile time. However, part of this is to complete the migration to `slf4j-api` at compile time, so the runtime implementations can be chosen by users. So, no actual implementation jar should need to be `compile` scoped anymore. They can just be `runtime` (or `test` if it is only needed to compile the tests; `runtime` implies `test`). The main point is that they aren't needed at `compile` time, because `slf4j-api` is used at compile time instead.
--
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
[GitHub] [zookeeper] cnauroth commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
cnauroth commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788278503
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
Reading through this...
https://infra.apache.org/licensing-howto.html#permissive-deps
...I don't think we're supposed to put the entire text of the EPL in here. Instead, it's typically a pointer to the dependency's license.
--
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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788520929
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
This one makes me confused: we didn't do this for the log4j1 reference. If you check it in this file, it's not "test" scoped. Also, if I make logback to "test" scoped, it won't be included in the binary distribution. Is that okay?
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1022031316
I've finished the last change that was requsted in the review. SLF4j version number is now just 1.7 in the admin docs.
Got the green light from @cnauroth as well, so @eolivelli if you're in the mood, launch 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
[GitHub] [zookeeper] eolivelli closed pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r778931407
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
If they are needed for tests, they could use the `test` scope explicitly here.
--
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
[GitHub] [zookeeper] Co-Din commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
Co-Din commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1033605285
Is the vote open to anyone to participate? If so where can we vote?
--
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789102065
##########
File path: zookeeper-server/pom.xml
##########
@@ -75,8 +75,12 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
`runtime` dependencies should still be included in the binary distribution... I'd have to check to see how you are constructing the binary distribution. But, you shouldn't put it on the `compile` class path just to put it in the binary distribution. There's better ways to include it in the distribution than changing the 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
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r788497962
##########
File path: LICENSE.txt
##########
@@ -200,3 +200,209 @@
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
+
Review comment:
My assumption was based on this guide: https://infra.apache.org/licensing-howto.html
> You must customize LICENSE and NOTICE files according to the content of the specific distribution they reside within. Do not add to LICENSE and NOTICE dependencies which are not in the distribution. **Only bundled bits matter.**
Even though logback is not included in our source distribution, we also have a convenience binary distribution with first-level dependencies included. But. Jetty falls into this bucket too which also has EPL v1.0 license and we don't include it in top-level files. So, I ended up reverting these files and add the logback license only to the resource folder.
--
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
[GitHub] [zookeeper] craiglawson commented on a change in pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
craiglawson commented on a change in pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#discussion_r789509507
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -504,22 +504,20 @@ The ZooKeeper service can be monitored in one of three primary ways:
### Logging
ZooKeeper uses **[SLF4J](http://www.slf4j.org)**
-version 1.7.5 as its logging infrastructure. For backward compatibility it is bound to
-**LOG4J** but you can use
-**[LOGBack](http://logback.qos.ch/)**
-or any other supported logging framework of your choice.
-
-The ZooKeeper default *log4j.properties*
-file resides in the *conf* directory. Log4j requires that
-*log4j.properties* either be in the working directory
+version 1.7.5 as its logging infrastructure. By default ZooKeeper is shipped with
Review comment:
1.7.30?
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1022125862
great work @anmolnar
committing as soon as CI is green
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1793: ZOOKEEPER-4427 Migrate to Logback
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1793:
URL: https://github.com/apache/zookeeper/pull/1793#issuecomment-1016790773
Almost there @eolivelli hold your last approval for a minute. ;)
I've submitted docs changes recently and would like to take care of the licenses file based on our LEGAL ticket. Stay tuned.
--
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