[GitHub] zookeeper issue #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on the issue: https://github.com/apache/zookeeper/pull/148 I'm still interested in it, though I'm not a Zookeeper committer, so I can't do anything about it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on the issue: https://github.com/apache/zookeeper/pull/148 If any transitive dependencies rely on the log4j 1.x API itself (not SLF4J or Commons Log or java.util.logging or an obscure bridge I didn't mention), then they can be replaced with log4j-1.2-api from Log4j2 in general unless they're defining custom appenders or other plugins. I'm not too familiar with Ivy, but here's what Gradle generates in its embedded ivy.xml for something similar: ```xml ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/148#discussion_r98037086
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -413,13 +418,18 @@ public void testBadPeerAddressInQuorum() throws
Exception {
ClientBase.setupTestEnv();
// setup the logger to capture all logs
+LoggerContext loggerContext = (LoggerContext)
LogManager.getContext(false);
--- End diff --
Oh sorry, I meant to get back to you on this much sooner. You can merge
without using it; just thought it would be a less hacky test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96442072 --- Diff: ivy.xml --- @@ -41,13 +41,20 @@ - + + + + + + + --- End diff -- At first I read that it was version 3.0.0, sorry. Anyways, log4j 2 tends to stick with the latest versions of dependencies, so I'd recommend 3.3.6. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96329244 --- Diff: ivy.xml --- @@ -41,13 +41,20 @@ - + + + + + + + + - + --- End diff -- Agreed, just pointing out in case anything is still using the old API. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/148#discussion_r96329273
--- Diff: conf/log4j2.xml ---
@@ -0,0 +1,56 @@
+
+
+
+
+%d{ISO8601} [myid:%X{myid}] -
%-5p [%t:%C{1}@%L] - %m%n
--- End diff --
Ok, makes sense. Would be something good to note somewhere, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145164 --- Diff: conf/log4j2.xml --- @@ -0,0 +1,56 @@ + + + --- End diff -- Not sure why you're using strict mode here. You're not using the right XML elements for strict mode. See the [docs](http://logging.apache.org/log4j/2.x/manual/configuration.html#Strict_XML). It'd be simpler to just remove the strict attribute as it's not the same type of strict as JS or Perl, for example (it's mainly useful for XSD validation). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/148#discussion_r96145218
--- Diff: conf/log4j2.xml ---
@@ -0,0 +1,56 @@
+
+
+
+
+%d{ISO8601} [myid:%X{myid}] -
%-5p [%t:%C{1}@%L] - %m%n
--- End diff --
I'd generally advise against [enabling location
information](http://logging.apache.org/log4j/2.x/manual/layouts.html#LocationInformation)
by default due to performance issues. I personally don't find location
information all that useful outside of caught exceptions anyways.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/148#discussion_r96145357
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -413,13 +418,18 @@ public void testBadPeerAddressInQuorum() throws
Exception {
ClientBase.setupTestEnv();
// setup the logger to capture all logs
+LoggerContext loggerContext = (LoggerContext)
LogManager.getContext(false);
--- End diff --
If this test is just storing log messages for verification at the end of
the test, then I recommend using log4j-core:test's `LoggerContextRule` and
`ListAppender`. Example:
[LoggerTest](https://github.com/apache/logging-log4j2/blob/master/log4j-core/src/test/java/org/apache/logging/log4j/core/LoggerTest.java),
[config.xml](https://github.com/apache/logging-log4j2/blob/master/log4j-core/src/test/resources/log4j-test2.xml)
(see the `` appender configs). This way, the test won't
break if internal details of log4j-core change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145281 --- Diff: src/docs/src/documentation/content/xdocs/zookeeperJMX.xml --- @@ -100,15 +100,15 @@ started the server you will be able to monitor and manage various service related features. -Also note that ZooKeeper will register log4j MBeans as +
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145251 --- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml --- @@ -618,23 +618,23 @@ server.3=zoo3:2888:3888 ZooKeeper uses http://www.slf4j.org;>SLF4J version 1.7.5 as its logging infrastructure. For backward compatibility it is bound to -LOG4J but you can use -http://logback.qos.ch/;>LOGBack +LOG4J2 but you can use --- End diff -- This paragraph doesn't make much sense anymore. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145267 --- Diff: src/docs/src/documentation/content/xdocs/zookeeperInternals.xml --- @@ -399,11 +399,11 @@ hierarchy of groups. Zookeeper uses http://www.slf4j.org/index.html;>slf4j as an abstraction layer for logging. -http://logging.apache.org/log4j;>log4j in version 1.2 is chosen as the final logging implementation for now. +http://logging.apache.org/log4j/2.x/;>log4j2 in version 2 is chosen as the final logging implementation for now. --- End diff -- Could you make this an https link? I've been trying to encourage the use of the https site. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145076 --- Diff: ivy.xml --- @@ -41,13 +41,20 @@ - + + + + + + + --- End diff -- Why are you using an older version of disruptor? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #148: ZOOKEEPER-2659 Log4j 2 migration
Github user jvz commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/148#discussion_r96145091 --- Diff: ivy.xml --- @@ -41,13 +41,20 @@ - + + + + + + + + - + --- End diff -- If anything is using log4j 1's API still, you'll need `log4j-1.2-api` as well. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
