César Álvarez Núñez
added a comment - 16/Apr/12 23:12
Removed Log4J dependencies from main source core.
Removed Log4J dependencies from test source core except for a bunch of test cases.
Modified ivy.xml and build.xml to make use of slf4j to log4j bridge only for testing purposes.
Modified build.xml to include a new test branch where no log4.jar is used.
Pending to review if documentacion needs to be updater.

Michi Mutsuzaki
added a comment - 17/Apr/12 01:13 Hi César,
The patch looks ok to me, but the unit test is failing. Could you take a look?
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034/testReport/org.apache.zookeeper.jmx/ManagedUtilNoLog4jTest/testZookeeperJmxLog4jDisableFalse/
Thanks!
--Michi

I'm not sure if the warning increment could be removed since it is due to replace Log4J references in ManagedUtil class by reflection in order to keep supporting Log4J JMX beans when "SLF to LOG4J" is used instead of a native slf4j implementation like LogBack.

Regarding to the failed tests, it seems that the build.xml modifications has not been applied. Is there some kind of mechanism that protect it?

César Álvarez Núñez
added a comment - 17/Apr/12 09:52 I'm not sure if the warning increment could be removed since it is due to replace Log4J references in ManagedUtil class by reflection in order to keep supporting Log4J JMX beans when "SLF to LOG4J" is used instead of a native slf4j implementation like LogBack.
Regarding to the failed tests, it seems that the build.xml modifications has not been applied. Is there some kind of mechanism that protect it?

Please update the "Logging" section of src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml. It should note that ZooKeeper uses slf4j and defaults to log4j binding (I'm assuming log4j-1.2.16.jar and slf4j-log4j12-1.6.4.jar will be included in the new release under lib/).

Right now bin/zkServer.
{sh,cmd} doesn't initialize logging properly for developers since log4j-1.2.16.jar and slf4j-log4j12-1.6.4.jar are located under build/test/lib/. Should we add build/test/lib/ to the classpath in bin/zkEnv.{sh,cmd}

Michi Mutsuzaki
added a comment - 26/Apr/12 08:40 Thanks for the patch, César! Here are my comments:
Please update the "Logging" section of src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml. It should note that ZooKeeper uses slf4j and defaults to log4j binding (I'm assuming log4j-1.2.16.jar and slf4j-log4j12-1.6.4.jar will be included in the new release under lib/).
Right now bin/zkServer.
{sh,cmd} doesn't initialize logging properly for developers since log4j-1.2.16.jar and slf4j-log4j12-1.6.4.jar are located under build/test/lib/. Should we add build/test/lib/ to the classpath in bin/zkEnv.{sh,cmd}
?
Thank you for adding unit tests for JMX!
--Michi

Hadoop QA
added a comment - 12/Jan/13 01:32 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12564532/ZOOKEEPER-1371.patch
against trunk revision 1427034.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 18 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1330//console
This message is automatically generated.

1.- Updated src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, but I can not verify it as doc files are copied from doc directory instead of being generated from docbook files. Am I missing something?

César Álvarez Núñez
added a comment - 12/Jan/13 01:36 1.- Updated src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, but I can not verify it as doc files are copied from doc directory instead of being generated from docbook files. Am I missing something?
2.- Included slf4j-log4j12-1.7.2.jar and log4j-1.2.17.jar build/lib directory.

Hadoop QA
added a comment - 12/Jan/13 02:02 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12564538/ZOOKEEPER-1371.patch
against trunk revision 1427034.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 18 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1331//console
This message is automatically generated.

Hadoop QA
added a comment - 13/Jan/13 19:12 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12564626/ZOOKEEPER-1371.patch
against trunk revision 1427034.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 18 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1333//console
This message is automatically generated.

Hadoop QA
added a comment - 13/Jan/13 19:32 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12564627/ZOOKEEPER-1371.patch
against trunk revision 1427034.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 18 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1334//console
This message is automatically generated.

César Álvarez Núñez
added a comment - 13/Jan/13 19:42 I don't understand why it is failing. I just checkout a clean trunk and I was able to successfully apply the patch. Trying a new patch without zooinspector/ivy.xml

1) ivy build files and dependencies, use same log4j version for testing scope.
2) I see no reason for why you want to include the log4j in distribution( ? ). It's only for running a (few) tests & I assume we don't want to dump the log4j into classpath for people. Whole point of slf4j is to let the developer who uses zookeeper to chose their choice of logging implementation of the slf4j api. Also ivy will pull down the log4j binding impl. for test scope for developers.
3) bumped log4j to version 1.2.17 (contrib/ivy declared dependency at version 1.2.15 which pulls in JMX SUN libraries which is not distributed in maven central. 1.2.16 was mentioned in root ivy file. Might as well bump to latest minor version.

Would be nice to see this merged now before we have to wait another X years

Roy Sindre Norangshol
added a comment - 01/Apr/14 21:35 Rebased existing patch and applied to master/trunk.
Also did a few changes:
1) ivy build files and dependencies, use same log4j version for testing scope.
2) I see no reason for why you want to include the log4j in distribution( ? ). It's only for running a (few) tests & I assume we don't want to dump the log4j into classpath for people. Whole point of slf4j is to let the developer who uses zookeeper to chose their choice of logging implementation of the slf4j api. Also ivy will pull down the log4j binding impl. for test scope for developers.
3) bumped log4j to version 1.2.17 (contrib/ivy declared dependency at version 1.2.15 which pulls in JMX SUN libraries which is not distributed in maven central. 1.2.16 was mentioned in root ivy file. Might as well bump to latest minor version.
Would be nice to see this merged now before we have to wait another X years

NoLog4j*.java"/>» which is no test files basically atm. I don't see why we would want to encourage users/devs to particular create such tests? So maybe it should be removed?

Camille Fournier: Can you confirm with packager that it is okey to drop log4j jar file from distribution? I see no reason for why you want it included in distribution as it then will land in classpath for users and that complicates things when they want to choose their own slf4j implementation for logging. I think it would be enough to have it declared as a test dependency in ivy for the log4j»-specific test implementations we have. This should be enough for developers I guess, maybe leave it as a note in a «readme.developers» if they want to run the jmx log4j test, they should fetch dependencies with ivy to ensure to get slf4j-log4j12 bridge for running it.

I guess it would make more sense to have the nolog4j goal to exclude the jmx log4j test instead, make it the default target for running tests and add a note in «readme.developers» to run «ant junit.run» to include tests which depends on log4j and then make CI to run the ant junit.run goal.

jmx log4j test is also an integration/system test as well, and most useful to be run by a CI.

Roy Sindre Norangshol
added a comment - 03/Apr/14 09:30 César Álvarez Núñez It's your patch, I'm unsure why you want the junit.run.nolog4j goal, it simply tries to only run the tests : «<include name="* / $
{test.category}
NoLog4j*.java"/>» which is no test files basically atm. I don't see why we would want to encourage users/devs to particular create such tests? So maybe it should be removed?
Camille Fournier : Can you confirm with packager that it is okey to drop log4j jar file from distribution? I see no reason for why you want it included in distribution as it then will land in classpath for users and that complicates things when they want to choose their own slf4j implementation for logging. I think it would be enough to have it declared as a test dependency in ivy for the log4j»-specific test implementations we have. This should be enough for developers I guess, maybe leave it as a note in a «readme.developers» if they want to run the jmx log4j test, they should fetch dependencies with ivy to ensure to get slf4j-log4j12 bridge for running it.
I guess it would make more sense to have the nolog4j goal to exclude the jmx log4j test instead, make it the default target for running tests and add a note in «readme.developers» to run «ant junit.run» to include tests which depends on log4j and then make CI to run the ant junit.run goal.
jmx log4j test is also an integration/system test as well, and most useful to be run by a CI.

I think it's fine to remove the log4j jar file, but at least the documentation needs to be updated. The commands shown in the documentation uses the log4j jar file included in the distribution. Maybe we can address this in a separate jira to get this one checked in?

As for the testing, my personal preference is to not exclude the jmx log4j tests. Ideally running "ant test" should run the same set of tests it did before this patch. Either that or modify the test so that it doesn't depend on log4j.

Michi Mutsuzaki
added a comment - 03/Apr/14 23:29 I think it's fine to remove the log4j jar file, but at least the documentation needs to be updated. The commands shown in the documentation uses the log4j jar file included in the distribution. Maybe we can address this in a separate jira to get this one checked in?
As for the testing, my personal preference is to not exclude the jmx log4j tests. Ideally running "ant test" should run the same set of tests it did before this patch. Either that or modify the test so that it doesn't depend on log4j.

What generates the docs/? I assume there is a tool or something that generates all the html/pdfs. Would be best to ensure docs are updated with the release.

Regarding the jmx log4j bean stuff, it should really be distributed as it's own artifact and then move the log4j specific implementation to this artifact together with the test. Because it doesn't really belong at all in the main project. Then users can contribute to other jmx logging artifacts for logback etc as well.

So I highly suggest to move it to its own artifact (jmx/log4j test we currently have), or at least move it to contrib and out of the main project.

Roy Sindre Norangshol
added a comment - 10/Apr/14 09:19 Michi Mutsuzaki Can probably create an issue with docs that is related to this one yes, if your thinking about -> http://zookeeper.apache.org/doc/r3.3.3/zookeeperAdmin.html#Debug+Log+Cleanup+%28log4j%29 etc.
What generates the docs/? I assume there is a tool or something that generates all the html/pdfs. Would be best to ensure docs are updated with the release.
Regarding the jmx log4j bean stuff, it should really be distributed as it's own artifact and then move the log4j specific implementation to this artifact together with the test. Because it doesn't really belong at all in the main project. Then users can contribute to other jmx logging artifacts for logback etc as well.
So I highly suggest to move it to its own artifact (jmx/log4j test we currently have), or at least move it to contrib and out of the main project.

Flavio Junqueira
added a comment - 10/Apr/14 09:52 What generates the docs/?
We use Forrest. You just have to update the files under src/docs/src/documentation/content/xdocs, and ideally run ant docs to make sure it builds alright.

Not directly related, but please have a note Log4j 2 reached RC1 and would be a great to choice to default too. It also has a (native) slf4j binding and brings an API with it too, if you want it. API supports various libs aswell. Check: http://logging.apache.org/log4j/2.x

Christian Grobmeier
added a comment - 10/Apr/14 20:16 Not directly related, but please have a note Log4j 2 reached RC1 and would be a great to choice to default too. It also has a (native) slf4j binding and brings an API with it too, if you want it. API supports various libs aswell. Check: http://logging.apache.org/log4j/2.x