ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1371

Remove dependency on log4j in the source code.

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.4.0, 3.4.1, 3.4.2, 3.4.3
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:

      Description

      ZOOKEEPER-850 added slf4j to ZK. We still depend on log4j in our codebase. We should remove the dependency on log4j so that we can make logging pluggable.

      1. ZOOKEEPER-1371.patch
        45 kB
        Roy Sindre Norangshol
      2. ZOOKEEPER-1371.patch
        73 kB
        César Álvarez Núñez
      3. ZOOKEEPER-1371.patch
        74 kB
        César Álvarez Núñez
      4. ZOOKEEPER-1371.patch
        45 kB
        César Álvarez Núñez

        Issue Links

          Activity

          Mahadev konar created issue -
          Mahadev konar made changes -
          Field Original Value New Value
          Link This issue is related to ZOOKEEPER-850 [ ZOOKEEPER-850 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12522869 ]
          Hide
          César Álvarez Núñez added a comment -
          • 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.
          Show
          César Álvarez Núñez added a comment - 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.
          César Álvarez Núñez made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 3.4.3 [ 12319288 ]
          Affects Version/s 3.4.2 [ 12319196 ]
          Affects Version/s 3.4.1 [ 12318650 ]
          Affects Version/s 3.4.0 [ 12314469 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12522869/ZOOKEEPER-1371.patch
          against trunk revision 1326029.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 18 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The applied patch generated 15 javac compiler warnings (more than the trunk's current 5 warnings).

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522869/ZOOKEEPER-1371.patch against trunk revision 1326029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 15 javac compiler warnings (more than the trunk's current 5 warnings). +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          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

          Show
          Michi Mutsuzaki added a comment - 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
          César Álvarez Núñez made changes -
          Assignee César Álvarez Núñez [ calvarez ]
          Hide
          César Álvarez Núñez added a comment -

          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?

          Show
          César Álvarez Núñez added a comment - 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?
          Hide
          Michi Mutsuzaki added a comment -

          Hmm, I'm not sure what happened. The build log says "patching file build.xml", but something could have gone wrong. I'll let somebody familiar with the build process answer.

          https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//consoleText

          Thanks!
          --Michi

          Show
          Michi Mutsuzaki added a comment - Hmm, I'm not sure what happened. The build log says "patching file build.xml", but something could have gone wrong. I'll let somebody familiar with the build process answer. https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1034//consoleText Thanks! --Michi
          Hide
          César Álvarez Núñez added a comment -

          My fault. New patch available.

          Show
          César Álvarez Núñez added a comment - My fault. New patch available.
          Hide
          César Álvarez Núñez added a comment -

          Fixed previous issues: warnings and failed test.

          Show
          César Álvarez Núñez added a comment - Fixed previous issues: warnings and failed test.
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12523050 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12522869 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12523050/ZOOKEEPER-1371.patch
          against trunk revision 1326029.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 18 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523050/ZOOKEEPER-1371.patch against trunk revision 1326029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1039//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          Thanks for the patch, César! Here are my comments:

          1. 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/).
          2. 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}

            ?

          3. Thank you for adding unit tests for JMX!

          --Michi

          Show
          Michi Mutsuzaki added a comment - 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
          Hide
          Patrick Hunt added a comment -

          Any updates on this? It would be a good time to clean this up.

          Show
          Patrick Hunt added a comment - Any updates on this? It would be a good time to clean this up.
          Hide
          César Álvarez Núñez added a comment -

          Patrick Hunt, quid pro quo. I update this one and you review ZOOKEEPER-1214

          Show
          César Álvarez Núñez added a comment - Patrick Hunt , quid pro quo. I update this one and you review ZOOKEEPER-1214
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564532 ]
          Hide
          Hadoop QA added a comment -

          -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.

          Show
          Hadoop QA added a comment - -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.
          Hide
          César Álvarez Núñez added a comment -

          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.

          Show
          César Álvarez Núñez added a comment - 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.
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564538 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564532 ]
          Hide
          Hadoop QA added a comment -

          -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.

          Show
          Hadoop QA added a comment - -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.
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564626 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564538 ]
          Hide
          Hadoop QA added a comment -

          -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.

          Show
          Hadoop QA added a comment - -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.
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564627 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564626 ]
          Hide
          Hadoop QA added a comment -

          -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.

          Show
          Hadoop QA added a comment - -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.
          Hide
          César Álvarez Núñez added a comment -

          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

          Show
          César Álvarez Núñez added a comment - 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
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564628 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12564628/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 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12564628/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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1335//console This message is automatically generated.
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564633 ]
          César Álvarez Núñez made changes -
          Attachment ZOOKEEPER-1371.patch [ 12564628 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12564633/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 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12564633/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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1336//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          The patch went stale. César, could you rebase and regenerate the patch?

          Thanks!
          --Michi

          Show
          Michi Mutsuzaki added a comment - The patch went stale. César, could you rebase and regenerate the patch? Thanks! --Michi
          Michi Mutsuzaki made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          César Álvarez Núñez added a comment -

          This patch is nearly 2 years old. It will take some time to refresh it

          Show
          César Álvarez Núñez added a comment - This patch is nearly 2 years old. It will take some time to refresh it
          Hide
          Michi Mutsuzaki added a comment -

          Yes, you are right. Sorry I've been putting this off for so long.

          Show
          Michi Mutsuzaki added a comment - Yes, you are right. Sorry I've been putting this off for so long.
          Hide
          Roy Sindre Norangshol added a comment -

          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

          Show
          Roy Sindre Norangshol added a comment - 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
          Roy Sindre Norangshol made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Labels patch
          Roy Sindre Norangshol made changes -
          Attachment ZOOKEEPER-1371.patch [ 12638125 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch
          against trunk revision 1583513.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch against trunk revision 1583513. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2018//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          +1 The patch looks good to me, thanks Roy. I'll give it a day for other people to review the patch and check it in.

          Show
          Michi Mutsuzaki added a comment - +1 The patch looks good to me, thanks Roy. I'll give it a day for other people to review the patch and check it in.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch
          against trunk revision 1583783.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch against trunk revision 1583783. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2019//console This message is automatically generated.
          Hide
          Roy Sindre Norangshol added a comment -

          Tests run locally just fine ( ant test-cppunit ), someone could probably try to rerun the patch again..

          Show
          Roy Sindre Norangshol added a comment - Tests run locally just fine ( ant test-cppunit ), someone could probably try to rerun the patch again..
          Show
          Michi Mutsuzaki added a comment - Restarted the build. https://builds.apache.org/view/S-Z/view/ZooKeeper/job/PreCommit-ZOOKEEPER-Build/2022/
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch
          against trunk revision 1583783.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch against trunk revision 1583783. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2022//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch
          against trunk revision 1583783.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch against trunk revision 1583783. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2024//console This message is automatically generated.
          Hide
          Camille Fournier added a comment -

          In general this looks good to me, should we just replace test.junit with test.junit-no-log4j (or whatever the target you made was called)?

          Show
          Camille Fournier added a comment - In general this looks good to me, should we just replace test.junit with test.junit-no-log4j (or whatever the target you made was called)?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch
          against trunk revision 1583783.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638125/ZOOKEEPER-1371.patch against trunk revision 1583783. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2025//console This message is automatically generated.
          Hide
          Roy Sindre Norangshol added a comment -

          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.

          Show
          Roy Sindre Norangshol added a comment - 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.
          Hide
          Michi Mutsuzaki added a comment -

          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.

          Show
          Michi Mutsuzaki added a comment - 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.
          Hide
          Roy Sindre Norangshol added a comment -

          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.

          Show
          Roy Sindre Norangshol added a comment - 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.
          Hide
          Flavio Junqueira added a comment -

          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.

          Show
          Flavio Junqueira added a comment - 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.
          Hide
          Michi Mutsuzaki added a comment -

          FYI ant docs didn't work for me with Forrest 0.9. I use Forrest 0.8.

          Show
          Michi Mutsuzaki added a comment - FYI ant docs didn't work for me with Forrest 0.9. I use Forrest 0.8.
          Hide
          Christian Grobmeier added a comment -

          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

          Show
          Christian Grobmeier added a comment - 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

            People

            • Assignee:
              César Álvarez Núñez
              Reporter:
              Mahadev konar
            • Votes:
              3 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:

                Development