Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.1
    • Fix Version/s: 3.4.0
    • Component/s: java client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      * replaces log4j with slf4j code (also in contrib for bookkeeper, zooinspector,rest,loggraph), added slf4j dependencies into several ivy.xml files
      * you must add slf4j-api-1.6.1.jar and slf4j-log4j12-1.6.1.jar (bridge from sl4j to log4j) to the classpath, if not using the standard scripts
      * log4j remains as the final logger yet, there is still work to do: remove programmatic access to the log4j api from certain classes (which add appenders or configure log4j at runtime), or move them to contrib

      Show
      * replaces log4j with slf4j code (also in contrib for bookkeeper, zooinspector,rest,loggraph), added slf4j dependencies into several ivy.xml files * you must add slf4j-api-1.6.1.jar and slf4j-log4j12-1.6.1.jar (bridge from sl4j to log4j) to the classpath, if not using the standard scripts * log4j remains as the final logger yet, there is still work to do: remove programmatic access to the log4j api from certain classes (which add appenders or configure log4j at runtime), or move them to contrib

      Description

      Hello,

      i would like to see slf4j integrated into the zookeeper instead of relying explicitly on log4j.

      slf4j is an abstract logging framework. There are adapters from slf4j to many logger implementations, one of them is log4j.

      The decision which log engine to use i dont like to make so early.

      This would help me to embed zookeeper in my own applications (which use a different logger implemenation, but slf4j is the basis)

      What do you think?

      (as i can see, those slf4j request flood all other projects on apache as well

      Maybe for 3.4 or 4.0?

      I can offer a patchset, i have experience in such an migration already.

      1. ZOOKEEPER-850.patch
        203 kB
        Patrick Hunt
      2. ZOOKEEPER-850.patch
        203 kB
        Michi Mutsuzaki
      3. ZOOKEEPER-850.patch
        206 kB
        Michi Mutsuzaki
      4. ZOOKEEPER-850.patch
        206 kB
        Olaf Krische
      5. ZOOKEEPER-3.4.0-log4j-slf4j-20101102.patch.bz2
        20 kB
        Olaf Krische
      6. ZOOKEEPER-3.3.1-log4j-slf4j-20101031.patch.bz2
        15 kB
        Olaf Krische

        Issue Links

          Activity

          Hide
          Patrick Hunt added a comment -

          I think it's reasonable, is it fully backward compatible? What are the implications/impact for end users (ie what will they have to change, will they see any changes in their output, etc...)?

          If you submit a patch we'd be able to evaluate better (try it out ourselves), feel free.

          Show
          Patrick Hunt added a comment - I think it's reasonable, is it fully backward compatible? What are the implications/impact for end users (ie what will they have to change, will they see any changes in their output, etc...)? If you submit a patch we'd be able to evaluate better (try it out ourselves), feel free.
          Hide
          Olaf Krische added a comment -

          Hello,

          if you still use the log4j logger at runtime, then it should have no impact on the logging output, since those definitions in log4j.properties for log4j remain valid.

          1)

          Add "slf4j-api-1.5.10.jar", its the API for slf4j

          2)

          For $clazz, replace:

          final static org.apache.log4j.Logger = org.apache.log4j.Logger.getLogger($clazz);

          with

          final static org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger($clazz);

          This must compile, then zookeeper is independend from log4j.

          3)

          For runtime, e.g. testing, decide which logger to use. For log4j, you would just add:

          • slf4j-log4j12-1.5.10.jar, the adapter from slf4j to log4j
          • log4j-1.2.15.jar for log4j, which should already be defined in ant

          This would be all.

          (patchset is not so easy as i thought, since zookeeper use its very own file structure, i dont even get 3.3.1 with a normal ant compile to work)

          Show
          Olaf Krische added a comment - Hello, if you still use the log4j logger at runtime, then it should have no impact on the logging output, since those definitions in log4j.properties for log4j remain valid. 1) Add "slf4j-api-1.5.10.jar", its the API for slf4j 2) For $clazz, replace: final static org.apache.log4j.Logger = org.apache.log4j.Logger.getLogger($clazz); with final static org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger($clazz); This must compile, then zookeeper is independend from log4j. 3) For runtime, e.g. testing, decide which logger to use. For log4j, you would just add: slf4j-log4j12-1.5.10.jar, the adapter from slf4j to log4j log4j-1.2.15.jar for log4j, which should already be defined in ant This would be all. (patchset is not so easy as i thought, since zookeeper use its very own file structure, i dont even get 3.3.1 with a normal ant compile to work)
          Hide
          Olaf Krische added a comment -

          So, in a way, i didnt change much. All what is required now is to have those two extra slf4j-jars in the classpath. Then it should run as always. Even if there is still a mistake, since log4j is still there, all should work as always.

          The "org.apache.zookeeper.jmx.ManagedUtil" has to go away. Why not as an extra utility in contrib? It manages log4j by jmx. So therefore it requires log4j. I didnt wanna do a structural change.

          You can remove "log4j" from ivy, then you will see the last dependencies, when building.

          ant test tar went thru without a problem.

          Show
          Olaf Krische added a comment - So, in a way, i didnt change much. All what is required now is to have those two extra slf4j-jars in the classpath. Then it should run as always. Even if there is still a mistake, since log4j is still there, all should work as always. The "org.apache.zookeeper.jmx.ManagedUtil" has to go away. Why not as an extra utility in contrib? It manages log4j by jmx. So therefore it requires log4j. I didnt wanna do a structural change. You can remove "log4j" from ivy, then you will see the last dependencies, when building. ant test tar went thru without a problem.
          Hide
          Olaf Krische added a comment -

          the patch

          Show
          Olaf Krische added a comment - the patch
          Hide
          Patrick Hunt added a comment -

          Hi Olaf, thanks for the patch. A couple questions/comments:

          1) can you create this patch against trunk? We only put bug fixes into the fix releases, so this would be slated for 3.4.0 release (current trunk).

          2) do any of the shell scripts need to be updated? (bin directory)

          3) I see references to log4j in the build.xml file(s). Do any of these need to be upated? It would be good if you could build a release (ant tar) and verify that the built archive can run zk server/client via the bin scripts.

          4) It looks like the documentation also needs to be updated, do a "egrep -Ri log4j src/docs/src/documentation/ log4j" from the toplevel. We should at least update the existing docs, also it would be helpful to include addl information to help both users and developers make the switch.

          5) we typically create "release notes" for a release, it would be good to document in this JIRA (the rel notes section) any details we should include in the release notes documentation that goes along with the release. Some short statement detailing the change an any impact (you've given some detail in the comments, basically wrapping it up into something short/simple for users to follow during upgrade).

          Thanks!

          Show
          Patrick Hunt added a comment - Hi Olaf, thanks for the patch. A couple questions/comments: 1) can you create this patch against trunk? We only put bug fixes into the fix releases, so this would be slated for 3.4.0 release (current trunk). 2) do any of the shell scripts need to be updated? (bin directory) 3) I see references to log4j in the build.xml file(s). Do any of these need to be upated? It would be good if you could build a release (ant tar) and verify that the built archive can run zk server/client via the bin scripts. 4) It looks like the documentation also needs to be updated, do a "egrep -Ri log4j src/docs/src/documentation/ log4j" from the toplevel. We should at least update the existing docs, also it would be helpful to include addl information to help both users and developers make the switch. 5) we typically create "release notes" for a release, it would be good to document in this JIRA (the rel notes section) any details we should include in the release notes documentation that goes along with the release. Some short statement detailing the change an any impact (you've given some detail in the comments, basically wrapping it up into something short/simple for users to follow during upgrade). Thanks!
          Hide
          Olaf Krische added a comment -

          Release-Notes:

          • replaces as far as possible the log4j with slf4j code (also in contrib for bookkeeper, zooinspector,rest,loggraph)
          • you must add slf4j-api-1.6.1jar and slf4j-log4j12-1.6.1 to the classpath!
          • log4j remains as the final logger, some code still depends directly on it (there is work to do...)
          Show
          Olaf Krische added a comment - Release-Notes: replaces as far as possible the log4j with slf4j code (also in contrib for bookkeeper, zooinspector,rest,loggraph) you must add slf4j-api-1.6.1jar and slf4j-log4j12-1.6.1 to the classpath! log4j remains as the final logger, some code still depends directly on it (there is work to do...)
          Hide
          Olaf Krische added a comment -

          @patrick

          1) done

          2) no

          3) i started zkServer/zkCli and it works with the bin scripts.

          About the build.xml files i am not really sure. For example, in ZooInspector it references an old zookeeper-3.3.0.jar. Broken anyways?

          Can someone look it up who is more familiar with the ant build system?

          4) i changed some of the "xml" files to reflect the change to slf4j. how to create the html files from it? they are not created when creating the tar.

          5) see the attachement

          Show
          Olaf Krische added a comment - @patrick 1) done 2) no 3) i started zkServer/zkCli and it works with the bin scripts. About the build.xml files i am not really sure. For example, in ZooInspector it references an old zookeeper-3.3.0.jar. Broken anyways? Can someone look it up who is more familiar with the ant build system? 4) i changed some of the "xml" files to reflect the change to slf4j. how to create the html files from it? they are not created when creating the tar. 5) see the attachement
          Hide
          Patrick Hunt added a comment -

          fyi hbase's jira for similar change: HBASE-2608 (looks like this patch is proposing similar to 2 in hbase jira?)

          Show
          Patrick Hunt added a comment - fyi hbase's jira for similar change: HBASE-2608 (looks like this patch is proposing similar to 2 in hbase jira?)
          Hide
          Patrick Hunt added a comment -

          Attaching Olaf's patch as raw patch file so that hudson can do it's magic.

          Show
          Patrick Hunt added a comment - Attaching Olaf's patch as raw patch file so that hudson can do it's magic.
          Hide
          Patrick Hunt added a comment -

          Olaf, re 5 could you add similar comments to this JIRA in the "release notes" section? We'll need that when doing the release itself.

          Show
          Patrick Hunt added a comment - Olaf, re 5 could you add similar comments to this JIRA in the "release notes" section? We'll need that when doing the release itself.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 249 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 appears to introduce 1 new Findbugs 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//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/12459170/ZOOKEEPER-850.patch against trunk revision 1033155. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 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 appears to introduce 1 new Findbugs 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/14//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/12459170/ZOOKEEPER-850.patch
          against trunk revision 1033155.

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

          +1 tests included. The patch appears to include 249 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 appears to introduce 1 new Findbugs 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//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/12459170/ZOOKEEPER-850.patch against trunk revision 1033155. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 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 appears to introduce 1 new Findbugs 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/21//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          The previous patch got broken after some checkins. I modified 2 files to fix compilation errors.

          • changed src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java to use LoggerFactory.getLogger().
          • changed fatal to error in src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java

          --Michi

          Show
          Michi Mutsuzaki added a comment - The previous patch got broken after some checkins. I modified 2 files to fix compilation errors. changed src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java to use LoggerFactory.getLogger(). changed fatal to error in src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java --Michi
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 249 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 appears to introduce 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//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/12465085/ZOOKEEPER-850.patch against trunk revision 1040752. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 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 appears to introduce 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/59//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          I still see some references to log4j. I'm not sure if they are expected. Olaf, could you comment?

          Thanks!
          --Michi

           
          src/java/test/org/apache/zookeeper/test/ClientBase.java:import org.apache.log4j.Priority;
          src/java/test/org/apache/zookeeper/test/ClientTest.java:import org.apache.log4j.Priority;
          src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig:import org.apache.log4j.Logger;
          src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Layout;
          src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Level;
          src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Logger;
          src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.WriterAppender;
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.LogManager;
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.jmx.HierarchyDynamicMBean;
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.spi.LoggerRepository;
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:     * Register the log4j JMX mbeans. Set environment variable
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:     * "zookeeper.jmx.log4j.disable" to true to disable registration.
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:        if (Boolean.getBoolean("zookeeper.jmx.log4j.disable") == true) {
          src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:        ObjectName mbo = new ObjectName("log4j:hiearchy=default");
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java:import org.apache.log4j.MDC;
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java:          LOG.warn("Unable to register log4j JMX control", e);
          src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java.orig:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java.orig:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java:            LOG.warn("Unable to register log4j JMX control", e);
          src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/ZooKeeper.java.orig:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/ClientCnxnSocket.java:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java:import org.apache.log4j.Logger;
          src/java/main/org/apache/zookeeper/ClientCnxn.java.orig:import org.apache.log4j.Logger;
          
          Show
          Michi Mutsuzaki added a comment - I still see some references to log4j. I'm not sure if they are expected. Olaf, could you comment? Thanks! --Michi src/java/test/org/apache/zookeeper/test/ClientBase.java:import org.apache.log4j.Priority; src/java/test/org/apache/zookeeper/test/ClientTest.java:import org.apache.log4j.Priority; src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig:import org.apache.log4j.Logger; src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Layout; src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Level; src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.Logger; src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:import org.apache.log4j.WriterAppender; src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.LogManager; src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.jmx.HierarchyDynamicMBean; src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java:import org.apache.log4j.spi.LoggerRepository; src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java: * Register the log4j JMX mbeans. Set environment variable src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java: * "zookeeper.jmx.log4j.disable" to true to disable registration. src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java: if (Boolean.getBoolean("zookeeper.jmx.log4j.disable") == true) { src/java/main/org/apache/zookeeper/jmx/ManagedUtil.java: ObjectName mbo = new ObjectName("log4j:hiearchy=default"); src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java:import org.apache.log4j.MDC; src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java: LOG.warn("Unable to register log4j JMX control", e); src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java.orig:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java.orig:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java: LOG.warn("Unable to register log4j JMX control", e); src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/ZooKeeper.java.orig:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/ClientCnxnSocket.java:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java:import org.apache.log4j.Logger; src/java/main/org/apache/zookeeper/ClientCnxn.java.orig:import org.apache.log4j.Logger;
          Hide
          Olaf Krische added a comment -

          Hello Michi,

          i have changed only the code, which uses log4j for logging.

          But some classes do more than that, they use log4j priorities, they add appenders at runtime, they do log4j business stuff. This logic i did not want to remove. It would be consequent to do so, but i thought, someone more experienced with the zookeeper project should do that.

          I commented that on the patch, that its incomplete.

          Help appreciated.

          Show
          Olaf Krische added a comment - Hello Michi, i have changed only the code, which uses log4j for logging. But some classes do more than that, they use log4j priorities, they add appenders at runtime, they do log4j business stuff. This logic i did not want to remove. It would be consequent to do so, but i thought, someone more experienced with the zookeeper project should do that. I commented that on the patch, that its incomplete. Help appreciated.
          Hide
          Michi Mutsuzaki added a comment -

          Thanks for the clarification, Olaf.

          This patch fixes the following files:

          src/java/test/org/apache/zookeeper/test/ClientBase.java
          src/java/test/org/apache/zookeeper/test/ClientTest.java
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
          src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
          src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
          

          I couldn't fix ManagedUtil.java and QuorumPeerMainTest.java because it uses log4j specific features. What should we do about these files?

          --Michi

          Show
          Michi Mutsuzaki added a comment - Thanks for the clarification, Olaf. This patch fixes the following files: src/java/test/org/apache/zookeeper/test/ClientBase.java src/java/test/org/apache/zookeeper/test/ClientTest.java src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java src/java/main/org/apache/zookeeper/client/StaticHostProvider.java src/java/main/org/apache/zookeeper/ClientCnxnSocket.java src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java I couldn't fix ManagedUtil.java and QuorumPeerMainTest.java because it uses log4j specific features. What should we do about these files? --Michi
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 249 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 appears to introduce 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//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/12465621/ZOOKEEPER-850.patch against trunk revision 1040752. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 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 appears to introduce 9 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/61//console This message is automatically generated.
          Hide
          Olaf Krische added a comment -

          @michi

          maven2 has its own test scope with its own dependency. so to select the "final" logger "log4j" in this test scope would cause no harm. can it be divided in ant projects as well?

          about managedutil, as far as i can see, it is jmx stuff. well, it should be moved to contrib or be deleted. everyone can write its own "jmx" handler for its "final" logging implementation, thats no biggie. just someone should made this decision.

          Show
          Olaf Krische added a comment - @michi maven2 has its own test scope with its own dependency. so to select the "final" logger "log4j" in this test scope would cause no harm. can it be divided in ant projects as well? about managedutil, as far as i can see, it is jmx stuff. well, it should be moved to contrib or be deleted. everyone can write its own "jmx" handler for its "final" logging implementation, thats no biggie. just someone should made this decision.
          Hide
          Patrick Hunt added a comment -

          ManagedUtil - this is adding log4j mbeans into the jmx registry so that log4j itself can be configured/monitored via jmx

          There should be something similar for slf4j, no?

          QuorumPeerMainTest - this allows us to verify particular messages are indeed being logged. We should be able to do similar with slf4j. no?

          Show
          Patrick Hunt added a comment - ManagedUtil - this is adding log4j mbeans into the jmx registry so that log4j itself can be configured/monitored via jmx There should be something similar for slf4j, no? QuorumPeerMainTest - this allows us to verify particular messages are indeed being logged. We should be able to do similar with slf4j. no?
          Hide
          Olaf Krische added a comment -

          @patrick

          there is no such for thing for slf4j. slf4j is just the logger interface, the api.

          you can configure the final logger by jmx, like log4j, sure. but which logger you use, this you can only know in your end project. as long as you are not "there", you can not configure anything. it makes no sense, since you just do logging in your code. thats why ManagedUtil could go to contrib, its log4j specific jmx stuff.

          in "tests" you will also chose a final logger, like log4j, "tests" are a final project. but this dependency should only be visible in tests. zookeeper itself should not deliver with log4j dependencies anymore.

          is it possible in ANT scripts to have a special test scope with test specific dependencies?

          Show
          Olaf Krische added a comment - @patrick there is no such for thing for slf4j. slf4j is just the logger interface, the api. you can configure the final logger by jmx, like log4j, sure. but which logger you use, this you can only know in your end project. as long as you are not "there", you can not configure anything. it makes no sense, since you just do logging in your code. thats why ManagedUtil could go to contrib, its log4j specific jmx stuff. in "tests" you will also chose a final logger, like log4j, "tests" are a final project. but this dependency should only be visible in tests. zookeeper itself should not deliver with log4j dependencies anymore. is it possible in ANT scripts to have a special test scope with test specific dependencies?
          Hide
          Mahadev konar added a comment -

          olaf,
          I dont think it would be wise to depend on both sl4fj and log4j. I'd really like us to move to sl4j (have heard a lot of good things abt it), but keeping a log4j dependency inthe project, just because we need it it one test and jmx would be weird. Neway we can get rid of log4j altogether?

          Show
          Mahadev konar added a comment - olaf, I dont think it would be wise to depend on both sl4fj and log4j. I'd really like us to move to sl4j (have heard a lot of good things abt it), but keeping a log4j dependency inthe project, just because we need it it one test and jmx would be weird. Neway we can get rid of log4j altogether?
          Hide
          Olaf Krische added a comment -

          i agree, totally. no log4j api access in the source code. still you need a logger impl in tests, like log4j.

          Show
          Olaf Krische added a comment - i agree, totally. no log4j api access in the source code. still you need a logger impl in tests, like log4j.
          Hide
          Mahadev konar added a comment -

          agreed. But we do have a code dependency on log4j in tests right? Do we want code dependency on log4j in tests? I am not an expert on loggin, so probably you'd be a better judge of this. What do you think?

          Show
          Mahadev konar added a comment - agreed. But we do have a code dependency on log4j in tests right? Do we want code dependency on log4j in tests? I am not an expert on loggin, so probably you'd be a better judge of this. What do you think?
          Hide
          Olaf Krische added a comment -

          In tests i would not mind any log4j api access, as long as this test and the decision to use log4j as logger is not carried out as a dependency in the published jars.

          Show
          Olaf Krische added a comment - In tests i would not mind any log4j api access, as long as this test and the decision to use log4j as logger is not carried out as a dependency in the published jars.
          Hide
          Mahadev konar added a comment -

          +1 the patch looks good. I will go ahead and commit after +1 from hudson.

          Show
          Mahadev konar added a comment - +1 the patch looks good. I will go ahead and commit after +1 from hudson.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/105//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/12465621/ZOOKEEPER-850.patch against trunk revision 1062244. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/105//console This message is automatically generated.
          Hide
          Michi Mutsuzaki (Inactive) added a comment -

          Hi Olaf,

          The patch doesn't apply any more. Could you update the patch if you have time? If not, I'll take care of it.

          Thanks!
          --Michi

          Show
          Michi Mutsuzaki (Inactive) added a comment - Hi Olaf, The patch doesn't apply any more. Could you update the patch if you have time? If not, I'll take care of it. Thanks! --Michi
          Hide
          Jan Høydahl added a comment -

          When is this scheduled for commit? We're about to release a new Solr version which faces issues with the log4j dependency.

          Show
          Jan Høydahl added a comment - When is this scheduled for commit? We're about to release a new Solr version which faces issues with the log4j dependency.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/149//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/12465621/ZOOKEEPER-850.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/149//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/12465621/ZOOKEEPER-850.patch
          against trunk revision 1072085.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/167//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/12465621/ZOOKEEPER-850.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/167//console This message is automatically generated.
          Hide
          Olaf Krische added a comment -

          Michi,

          i am currently kind of flooded with other stuff. Maybe on a weekend i will try to create a new patch again.

          Show
          Olaf Krische added a comment - Michi, i am currently kind of flooded with other stuff. Maybe on a weekend i will try to create a new patch again.
          Hide
          Mahadev konar added a comment -

          cancelling the patch for now.

          Show
          Mahadev konar added a comment - cancelling the patch for now.
          Hide
          Olaf Krische added a comment -

          updated patch

          Show
          Olaf Krische added a comment - updated patch
          Hide
          Olaf Krische added a comment -

          updated patch

          Show
          Olaf Krische added a comment - updated patch
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 249 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 appears to introduce 1 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//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/12472757/ZOOKEEPER-850.patch against trunk revision 1074995. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 249 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 appears to introduce 1 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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/179//console This message is automatically generated.
          Hide
          Michi Mutsuzaki (Inactive) added a comment -

          +1.

          There is one findbugs warning, but it's not this patch that introduced it.

          ===
          Exception is caught when Exception is not thrown in org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread.run()

          Bug type REC_CATCH_EXCEPTION (click for details)
          In class org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread
          In method org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread.run()
          At QuorumPeer.java:[line 310]

          Show
          Michi Mutsuzaki (Inactive) added a comment - +1. There is one findbugs warning, but it's not this patch that introduced it. === Exception is caught when Exception is not thrown in org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread.run() Bug type REC_CATCH_EXCEPTION (click for details) In class org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread In method org.apache.zookeeper.server.quorum.QuorumPeer$ResponderThread.run() At QuorumPeer.java: [line 310]
          Hide
          John Wang added a comment -

          This is becoming more urgent as oracle has removed com.sun.jmx and com.sun.jdmk packages from maven central... The reason it matters for this is because log4j 1.2.15 depends on those classes.

          So on a fresh build for zookeeper or any thing that depends on zookeeper via maven would not work...

          Show
          John Wang added a comment - This is becoming more urgent as oracle has removed com.sun.jmx and com.sun.jdmk packages from maven central... The reason it matters for this is because log4j 1.2.15 depends on those classes. So on a fresh build for zookeeper or any thing that depends on zookeeper via maven would not work...
          Hide
          Mahadev konar added a comment -

          @John,
          Yup this definitely is a issue. What I usually do is the following in my pom files:

            <dependency>
                <groupId>org.apache.hadoop</groupId>
                <artifactId>zookeeper</artifactId>
                <version>3.3.1</version>
                <scope>compile</scope>
                 <exclusions>
                  <exclusion>
                    <groupId>com.sun.jdmk</groupId>
                    <artifactId>jmxtools</artifactId>
                  </exclusion>
                  <exclusion>
                    <groupId>com.sun.jmx</groupId>
                    <artifactId>jmxri</artifactId>
                  </exclusion>
                </exclusions>
              </dependency>
          

          This works for me but should obviously be fixed.

          Show
          Mahadev konar added a comment - @John, Yup this definitely is a issue. What I usually do is the following in my pom files: <dependency> <groupId>org.apache.hadoop</groupId> <artifactId>zookeeper</artifactId> <version>3.3.1</version> <scope>compile</scope> <exclusions> <exclusion> <groupId>com.sun.jdmk</groupId> <artifactId>jmxtools</artifactId> </exclusion> <exclusion> <groupId>com.sun.jmx</groupId> <artifactId>jmxri</artifactId> </exclusion> </exclusions> </dependency> This works for me but should obviously be fixed.
          Hide
          Patrick Hunt added a comment -

          Is this one ready to go? We really should try to get this in, thanks for sticking with us Olaf!

          Show
          Patrick Hunt added a comment - Is this one ready to go? We really should try to get this in, thanks for sticking with us Olaf!
          Hide
          Michi Mutsuzaki added a comment -

          I think it's good to go. Mahadev and I +1ed it.

          --Michi

          Show
          Michi Mutsuzaki added a comment - I think it's good to go. Mahadev and I +1ed it. --Michi
          Hide
          Patrick Hunt added a comment -

          Michi – In that case why don't you do the honors! (commit it to trunk)

          Show
          Patrick Hunt added a comment - Michi – In that case why don't you do the honors! (commit it to trunk)
          Hide
          Michi Mutsuzaki added a comment -

          Ok, I'll give it a try.

          --Michi

          Show
          Michi Mutsuzaki added a comment - Ok, I'll give it a try. --Michi
          Hide
          Olaf Krische added a comment -

          A little more confidence here! If there is anything happening, it can be fixed. Thumbs up!

          (please do not forget the sub-task...)

          Show
          Olaf Krische added a comment - A little more confidence here! If there is anything happening, it can be fixed. Thumbs up! (please do not forget the sub-task...)
          Hide
          Michi Mutsuzaki added a comment -

          Checked into trunk.

          Olaf, thank you for the patch!
          --Michi

          Show
          Michi Mutsuzaki added a comment - Checked into trunk. Olaf, thank you for the patch! --Michi
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1151 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/1151/)
          ZOOKEEPER-850: Switch from log4j to slf4j (Olaf Krische via michim)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1151 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/1151/ ) ZOOKEEPER-850 : Switch from log4j to slf4j (Olaf Krische via michim)
          Hide
          Olaf Krische added a comment -

          This task can not be closed yet, until subtask ZOOKEEPER-1010 has been resolved. Otherwise it will always crash, when someone starts the client up and has the log4j dependency removed.

          Show
          Olaf Krische added a comment - This task can not be closed yet, until subtask ZOOKEEPER-1010 has been resolved. Otherwise it will always crash, when someone starts the client up and has the log4j dependency removed.
          Hide
          Michi Mutsuzaki added a comment -

          Oh yeah. I'll leave this open until ZOOKEEPER-1010 is resolved.

          --Michi

          Show
          Michi Mutsuzaki added a comment - Oh yeah. I'll leave this open until ZOOKEEPER-1010 is resolved. --Michi
          Hide
          Mahadev konar added a comment -

          Olaf/Michi,
          Is ZOOKEEPER-1010 a blocker for 3.4. release? My understanding is that folks still need to use log4j since the testing is dependent on it. I dont think that should be blocker. Please correct me if I am wrong.

          thanks

          Show
          Mahadev konar added a comment - Olaf/Michi, Is ZOOKEEPER-1010 a blocker for 3.4. release? My understanding is that folks still need to use log4j since the testing is dependent on it. I dont think that should be blocker. Please correct me if I am wrong. thanks
          Hide
          Holger Hoffstätte added a comment -

          Why would log4j be required for "testing"? That's a bug. The whole problem is that you're tying "management" functionality to your logging library, and that needs to be removed (or more precisely separated). The coupling of ManagedUtil to log4j is a blocker for resolving this issue. It may not be relevant for running the server standalone, but it is a HUGE problem for running the server "embedded" (e.g. in an app server) just as it is for the "client-only" side of things. Which of course is just another case for separating the two in the first place.

          Show
          Holger Hoffstätte added a comment - Why would log4j be required for "testing"? That's a bug . The whole problem is that you're tying "management" functionality to your logging library, and that needs to be removed (or more precisely separated). The coupling of ManagedUtil to log4j is a blocker for resolving this issue. It may not be relevant for running the server standalone, but it is a HUGE problem for running the server "embedded" (e.g. in an app server) just as it is for the "client-only" side of things. Which of course is just another case for separating the two in the first place.
          Hide
          Olaf Krische added a comment -

          It is not a blocker. As long as log4j remains as a dependency in the distribution, zookeeper runs fine. We just call slf4j functions, which themselves call log4j functions, instead of calling log4j directly.

          In any way, the bug is now a year old. Who uses ManagedUtil anyways? It is not my code, i do not have the guts to just kick it out or move it away.

          Show
          Olaf Krische added a comment - It is not a blocker. As long as log4j remains as a dependency in the distribution, zookeeper runs fine. We just call slf4j functions, which themselves call log4j functions, instead of calling log4j directly. In any way, the bug is now a year old. Who uses ManagedUtil anyways? It is not my code, i do not have the guts to just kick it out or move it away.
          Hide
          Mahadev konar added a comment - - edited

          Sadly,
          Its used in ZookeeperServerMain and QuorumPeerConfig.java. I do not want to block the release on these changes. I'd rather have something that works in 3.4.0 and remove the remaining part in ZOOKEEPER-1010 in 3.4.1. Would that be acceptable?

          Show
          Mahadev konar added a comment - - edited Sadly, Its used in ZookeeperServerMain and QuorumPeerConfig.java. I do not want to block the release on these changes. I'd rather have something that works in 3.4.0 and remove the remaining part in ZOOKEEPER-1010 in 3.4.1. Would that be acceptable?
          Hide
          Olaf Krische added a comment -

          To me it is acceptable.

          Show
          Olaf Krische added a comment - To me it is acceptable.
          Hide
          Olaf Krische added a comment -

          @Holger

          For testing it is ok to depend on log4j. I must log somehow, right? Especially if i test on cli, when there is no runtime like eclipse, which could pass me a default logger.

          But i might agree with you, that it is a bug, when zookeeper subprojects like client, server and tests are bound together so tight, that test dependencies, like logging with log4j, are carried out to server and client as well, just because there is no special context for testing. I was wondering this on my comment from 07/Dec/10 as well. In Maven i am used to have a test context. But is it possible with ant scripts?

          Show
          Olaf Krische added a comment - @Holger For testing it is ok to depend on log4j. I must log somehow, right? Especially if i test on cli, when there is no runtime like eclipse, which could pass me a default logger. But i might agree with you, that it is a bug, when zookeeper subprojects like client, server and tests are bound together so tight, that test dependencies, like logging with log4j, are carried out to server and client as well, just because there is no special context for testing. I was wondering this on my comment from 07/Dec/10 as well. In Maven i am used to have a test context. But is it possible with ant scripts?
          Hide
          César Álvarez Núñez added a comment -

          Hi all,

          Calls to org.slf4j.Logger methods should use parameters instead of "string concatenation".
          That's one of the main reasons to use SLF4J. More information at: http://www.slf4j.org/faq.html#logging_performance

          Show
          César Álvarez Núñez added a comment - Hi all, Calls to org.slf4j.Logger methods should use parameters instead of "string concatenation". That's one of the main reasons to use SLF4J. More information at: http://www.slf4j.org/faq.html#logging_performance
          Hide
          Olaf Krische added a comment -

          @cesar, do you have examples?

          i did only a replace of already existing logging calls, so log.debug or log.info stuff should still be surrounded by log.isDebugEnabled or log.isInfoEnabled as in the original. well, and about warnings and errors, we have chosen an older slf4j api (<1.6) so you still have to do a string concat for customizing your log message, only this enables you to log the stacktrace of an exception.

          so i think this is just details. i wish someone could just remove the direct log4j dependencies. i can not do this.

          and after that we can change to a newer slf4j api and make the beauty work in the source code.

          Show
          Olaf Krische added a comment - @cesar, do you have examples? i did only a replace of already existing logging calls, so log.debug or log.info stuff should still be surrounded by log.isDebugEnabled or log.isInfoEnabled as in the original. well, and about warnings and errors, we have chosen an older slf4j api (<1.6) so you still have to do a string concat for customizing your log message, only this enables you to log the stacktrace of an exception. so i think this is just details. i wish someone could just remove the direct log4j dependencies. i can not do this. and after that we can change to a newer slf4j api and make the beauty work in the source code.
          Hide
          Olaf Krische added a comment -

          i can not change the source code with the direct log4j dependencies. unless someone from the project decides: lets just remove this, no one needs it anyways!

          Show
          Olaf Krische added a comment - i can not change the source code with the direct log4j dependencies. unless someone from the project decides: lets just remove this, no one needs it anyways!
          Hide
          Mahadev konar added a comment -

          Moving it out of 3.4

          Show
          Mahadev konar added a comment - Moving it out of 3.4
          Hide
          Jean-Daniel Cryans added a comment -

          I think this jira should be resolved and a new one opened since code committed here is already part of a release. More so that the release note contains this:

          you must add slf4j-api-1.6.1.jar and slf4j-log4j12-1.6.1.jar (bridge from sl4j to log4j) to the classpath, if not using the standard scripts

          and that it doesn't appear anywhere since it wasn't resolved. BTW this creeped out as a new HBase client dependency too.

          Show
          Jean-Daniel Cryans added a comment - I think this jira should be resolved and a new one opened since code committed here is already part of a release. More so that the release note contains this: you must add slf4j-api-1.6.1.jar and slf4j-log4j12-1.6.1.jar (bridge from sl4j to log4j) to the classpath, if not using the standard scripts and that it doesn't appear anywhere since it wasn't resolved. BTW this creeped out as a new HBase client dependency too.
          Hide
          Mahadev konar added a comment -

          Marking this as fixed for 3.4.0. Will create a new jira for removing the actual dependency on log4j.

          Show
          Mahadev konar added a comment - Marking this as fixed for 3.4.0. Will create a new jira for removing the actual dependency on log4j.
          Hide
          Jean-Daniel Cryans added a comment -

          Thanks Mahadev!

          Show
          Jean-Daniel Cryans added a comment - Thanks Mahadev!
          Hide
          Shikhar Bhushan added a comment -

          What's the ticket for fixing the dependency?

          Show
          Shikhar Bhushan added a comment - What's the ticket for fixing the dependency?
          Hide
          Shikhar Bhushan added a comment -

          Found it under issue links: ZOOKEEPER-1371

          Show
          Shikhar Bhushan added a comment - Found it under issue links: ZOOKEEPER-1371

            People

            • Assignee:
              Olaf Krische
              Reporter:
              Olaf Krische
            • Votes:
              10 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development