You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/25 19:08:40 UTC

[GitHub] [bookkeeper] Ghatage opened a new pull request #2455: BP-40: Convert all System.*.print statements to LOG.* statements

Ghatage opened a new pull request #2455:
URL: https://github.com/apache/bookkeeper/pull/2455


   ### Motivation
   
   In order to have an audit log of all user interactions, all print statements which come from such interactions need to be logged.
   We have two user driven ways to mutate BookKeeper:
   * BookieShell
   * bkctl
   Both internally use the same code from `tools/cli`
   
   ### Changes
   
   The logging in the code was inconsistent. At times we used `System.out.println` and at times we used `LOG.info`.
   As a part of this work item we change these in the following way:
   
   * For every `cli` class, confirm if a Logger object is present, if not create one.
   * Convert all `System.out.println()` -> `LOG.info()`
   * Convert all `System.err.println()` -> `LOG.error()`
   
   ### Ramifications
   
   BookieShell uses `log4j.shell.properties`
   bkctl uses `log4j.cli.properties`
   
   Both log4j property files have `CONSOLE appenders`, so the like previous `System.out.println()`, the new `LOG` statements will also be printed to console.
   This is mentioned explicitly as certain upstream consumers / scripts utilize BookieShell / bkctl to assess the state of the BK cluster.
   
   Master Issue: #2209 
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [bookkeeper] Ghatage commented on pull request #2455: BP-40: Convert all System.*.print statements to LOG.* statements

Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2455:
URL: https://github.com/apache/bookkeeper/pull/2455#issuecomment-716717906


   Hi @eolivelli,
   
   Apologies as I think I wasn't clear. Let me explain.
   
   1. BookieShell / bkctl already use Loggers and print to a log file **as well as** to the console.
   For example in [this command](https://github.com/apache/bookkeeper/blob/aff51a53dbe38470c9c5f80954210af56965e73b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LocalConsistencyCheckCommand.java#L61) like many others already we have `LOG.info()` which prints to the console as well as to the log file.
   
   2. So now we know that when we execute these interactive commands, we print to the console as well as log to a file.
   The problem arises when there is inconsistency. 
   From my changes you can see that there are places where we have `System.out.println()` instead of `LOG.info()`
   So in those cases, we **only print to the console** and don't save that interaction output in the log file.
   
   This change relieves that inconsistency and makes all commands print to console **as well as** log file.
   This way, when we implement audit logging, it will save all user interactions.
   
   
   > How can we distinguish the output for the user from the logs of the application ?
   
   We currently log bookieshell logs differently in a different log file.
   So interactions from the user (bookieshell/bkctl) will go into that log file.
   The logs of the server are in a separate log file.
   Log file name, location and access properties for bkctl are controlled by [log4j.cli.properties](https://github.com/apache/bookkeeper/blob/master/conf/log4j.cli.properties)
   and log file name, location and other properties for BookieShell are controlled by [log4j.shell.properties](https://github.com/apache/bookkeeper/blob/master/conf/log4j.shell.properties) 
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [bookkeeper] Ghatage commented on pull request #2455: BP-40: Convert all System.*.print statements to LOG.* statements

Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2455:
URL: https://github.com/apache/bookkeeper/pull/2455#issuecomment-716197096


   @zymap also PTAL since you have done lots of work in this area!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [bookkeeper] eolivelli commented on pull request #2455: BP-40: Convert all System.*.print statements to LOG.* statements

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2455:
URL: https://github.com/apache/bookkeeper/pull/2455#issuecomment-716496303


   I am not sure this is good.
   I have to think about it a bit more.
   How can we distinguish the output for the user from the logs of the application ?
   
   The logger usually is something that writes to a file useful information, it is not part of the UI (the stuff the user sees)
   
   for instance, if I want to ask for a username I am not going to use
   log.info("Please tell me the username:")
   console.readLine();
   
   but
   console.println("Please tell me the username:")
   console.readLine();
   
   these tool are interactive, I am not sure the log system is what we want to use


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [bookkeeper] eolivelli merged pull request #2455: BP-40: Convert all System.*.print statements to LOG.* statements

Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2455:
URL: https://github.com/apache/bookkeeper/pull/2455


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org