SourceForge has been redesigned. Learn more.
Close

#53 Move to log4j for logging.

closed-accepted
nobody
None
5
2008-04-29
2008-04-22
No

This patch changes the code to support log4j logging. Please review and let me know if there are any comments/questions. I highly suggest you try running with this patch and make sure you like the behavior (we can easily change the formatting of the log messages themselves - you can play with this by changing the log4j properties file).

I'm doing this cutover in stages. This first stage is to get
log4j integrated and move most of the code over to log4j. Partly to break
things up into more manageable (mergeable) chunks but also partly so that
ppl can try things out and we can more easily change if there are issues.

Also note: the originally code almost exclusivley used "logError" and
"logWarn", there didn't seem to be a notion of "info" level messages. In
this first pass I mapped these calls to LOG.error and LOG.warn respectively.
We MUST go through the code and update the calls to accurately reflect
the log level intended. I plan to do a pass on this during the next
stage of migration.

Also note: the original code had the notion of tracing as well as error/warn
logging. I've kept the tracing as-is for the time being. I will migrate
this code in a future stage as well.

To apply this patch:

1) unarchive the attached archive file w/in the zookeeper top level directory
2) apply the patch contained in the tar file
3) remove log4j-1.2.9.jar from java/lib

Notes:

1) there are 2 new shell scripts in bin directory, one for running
the server and one for running the client shell.

App startup now has more options -- having these scripts will simplify
the onboarding process (new users). We will need to update the docs on
the twiki to reflect this change.

2) zookeeper-dev.jar is now generated into java/lib directory

3) the server default log4j config has 2 appenders - one to the console and
one to a file. Notice that the console does not include the date (but
we could change the config file if this is considered important to have
as default).

We need to document log4j configuration in zk on our twiki.

The client log4j config only outputs to the console.

Discussion

  • Patrick Hunt

    Patrick Hunt - 2008-04-22

    Logged In: YES
    user_id=12853
    Originator: YES

    SF will not allow me to upload log4j jar file version 1.2.15. You will need to
    download the latest log4j archive, extract log4j-1.2.15.jar and put it into the
    java/lib directory (remove 1.2.9).

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-23

    Logged In: YES
    user_id=154690
    Originator: NO

    Great work Pat! We need to decide if we want to apply this now or wait until after the next release. I'm for getting it in ASAP.

    I'm not sure about the usefulness of the scripts. They are pretty simple and make assumptions about directory layout and the current directory when the script is run. I'd rather have a README that has those two lines in it as an example.

    It seems weird to have zookeeper-dev.jar end up in the lib directory. If anything it should really end up in the build directory, but it's also logical to have it in the same directory as where you run ant. (At least it's the first place you look :)

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-23

    Logged In: YES
    user_id=12853
    Originator: YES

    Why's it called zookeeper-dev.jar rather than just zookeeper.jar?

    I'd vote for moving it into build directory.

    There's just alot more to type now. ;-)
    The idea of the script is to make it dead simple to d/l and try out zookeeper. One problem I noticed with adding
    log4j.jar to the classpath is that I couldn't get the simpler "-jar zookeeper.jar" command line to work. Adding
    classpath to the manifest doesn't seem to be a solution.

    I'm personally fine w/o the scripts - but wanted to throw it out there in terms of simplifying the "new user
    experience".

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-25

    Logged In: YES
    user_id=154690
    Originator: NO

    Since the scripts and the change in the build are orthogonal to this issue lets put them in a separate patch. (Not totally orthogonal because as you point out you can't run with java -jar because of the external dependency. There are ways to take care of that but I'd like to make that a separate issue.)

    I'd like to get this in ASAP.

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-25

    Logged In: YES
    user_id=12853
    Originator: YES

    File Added: log4j.patch

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-25

    log4j patch file

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-25

    Logged In: YES
    user_id=12853
    Originator: YES

    I have updated the patch to:
    1) all changes are in 1 file (except log4j-1.2.15.jar which is too large and you still need to get from log4j site)
    2) removed server/shell scripts from patch (will submit separately after discussion)
    3) removed change to build - not moving the output directory for the zk jar

    AFAIK this patch is ready for commit - please review/vote.

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-26

    Logged In: YES
    user_id=154690
    Originator: NO

    +1 I'll commit this.

    BTW, we already have log4j in the lib directory. Do we need both version, or can we just go with the newest?

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    Logged In: YES
    user_id=12853
    Originator: YES

    We don't need 1.2.9, just remove it (I verified cobertura likes 1.2.15)

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    readme with log4j details.

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    Logged In: YES
    user_id=12853
    Originator: YES

    I realized I missed the README in this patch. I added a readme to detail how to run the client/server with the new dependency of log4j. Please feel free to edit as appropriate.
    File Added: README

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-28

    Logged In: YES
    user_id=1926680
    Originator: NO

    should be have something like
    bin/zookeeper start server which does all the stuff mentioned in README?

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    Logged In: YES
    user_id=12853
    Originator: YES

    I had that in the original patch but Ben had me remove it. I think there's another pending patch that has a sample init.d script.

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-28

    Logged In: YES
    user_id=1926680
    Originator: NO

    also, are we including these config files in the jar? so in case if a client is running with zookeeper.jar in the classpath, then he does not need to add anything else to get the client library working.

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-28

    Logged In: YES
    user_id=1926680
    Originator: NO

    to correct my last comment log4j and zookeeper.jar will be the only ones needed to add to the classpath.

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    Logged In: YES
    user_id=12853
    Originator: YES

    I don't think we should include the configs in the jar since users would have to modify the jar to change their config. Personally I like the external file w/commandline better. That's why I provided the scripts originally.

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-28

    Logged In: YES
    user_id=1926680
    Originator: NO

    thye dont really have to modify the jar. In case they want to change it they will have it as the first element in the classpath. So we pick that one up rather than the one in the jar.

     
  • Patrick Hunt

    Patrick Hunt - 2008-04-28

    Logged In: YES
    user_id=12853
    Originator: YES

    That's true.

    I had console+rollingappender for the log4j default config.

    If we do this we should comment out the rolling appender (leave it there as sample) and make console logging by default. I only have INFO level and higher going to the console.

    What do you think?

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-28

    Logged In: YES
    user_id=1926680
    Originator: NO

    +1 makes sense to me.

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-28

    Logged In: YES
    user_id=154690
    Originator: NO

    I think it may be problematic to put the config file in the jar. If the classpath is the following:
    zookeeper-dev.jar:java/lib/log4j-1.2.15.jar:conf

    and zookeeper-dev.jar has the log4j properties, then any log4j properties in conf will be ignored. If we add a fat jar to the build, we should put in the log4j properties there.

    For now, I think we should commit the patch as it is.

     
  • Mahadev Konar

    Mahadev Konar - 2008-04-29

    Logged In: YES
    user_id=1926680
    Originator: NO

    ok lets get it in first without the configs in jars and later we can see what to do with it.

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-29

    Logged In: YES
    user_id=154690
    Originator: NO

    Committed revision 137.

     
  • Benjamin Reed

    Benjamin Reed - 2008-04-29
    • status: open --> closed-accepted
     

Log in to post a comment.