FLUME-283: Start switching to slf4j

Review Request #1059 — Created Oct. 21, 2010 and submitted

bruce.mitchener
old-flume
FLUME-283
flume
Start changing over to the SLF4J API. This only modifies the "easy" files, not the ones where we are actually using Log4J API stuff that isn't compatible with the SLF4J API.

This does build and should be ready to be committed.

Subsequent patches will address the remaining files that make use of problematic parts of the Log4J API.
Tests appear to run as normal for me and populate logs.
jon
  1. will put more comments in jira: https://issues.cloudera.org/browse/FLUME-283
  2. Lots of dupes?
  3. duplicates?
  4. 
      
phunt
  1. FYI "ant eclipse" will generate the eclipse project files necessary to develop flume in eclipse.
    ctrl-shift-O (oh) will "organize imports" and automagically fix this type of issue.
    1. btw, this works at not only the file level but also the package and above level - you can clean up all the source with one keystroke. All hail eclipse!
  2. 
      
bruce.mitchener
phunt
  1. lgtm with the noted comments. Also:
    
    1) do the user docs need to be updated (logging configuration)? are there user docs? Best practices, what is our default, how to change config, what to expect, etc...
    
    I'd suggest creating a jira (if not one already) requiring documentation to be created for this. That jira should be next on the list (after committing this, or you could just create the doc as part of this review if you like)
    
    2) document developer best practices for logging (again, another jira would be ok). Maybe the wiki is a good place for this. esp highlight some of the features avail in slf4 (formatting in particular comes to mind)
    
    3) you're doing alot of manual checking that could easily be automated with checkstyle (pmd?). I'd suggest that you enable checkstyle in ant build.xml, give it a very limited default set of checks (private/protected static final LOG fields for example)
    
    4) It's unfortunate that the tests are hardcoded to override the logging levels. I see why Jon is doing this, but if there were a better way to do this (configuration at the test level) I'd be happier. Not sure if slf4j helps with this.
    1. It seems like the majority think that the way I've been using Level in these cases isn't kosher, so if you think it is better to get rid of it I'm ok with it.  Maybe at the least keep comments with the classes/levels that make debugging the particular tests easier?
  2. Any reason why we wouldn't make all the LOG fields private? (protected perhaps?)
    
    Also, all the LOG fields should be final.
    (I won't check/comment every case here, you should grep for it (whatever)). 
  3. this should be static, no? 
    
    I won't check/comment for all cases, but you should grep for this and verify that all are static.
  4. 
      
bruce.mitchener
Review request changed

Description:

~  

Start changing over to slf4j. This doesn't build yet. See bug for some notes / comments.

  ~

Start changing over to the SLF4J API. This only modifies the "easy" files, not the ones where we are actually using Log4J API stuff that isn't compatible with the SLF4J API.

  +
  +

This does build and should be ready to be committed.

  +
  +

Subsequent patches will address the remaining files that make use of problematic parts of the Log4J API.

Testing Done:

~  

None yet.

  ~

Tests appear to run as normal for me and populate logs.

jon
  1. lgtm.  Thanks for all this work!
    
    We dicussed in IRCm  I agree that that almost all of these should be definitely not be public and either private static final or package static final.  We can make this a new jira that should be marked "easy"/trivial.  I marked a bunch of places where finals were missed for whomever wants to follow up on this. 
  2. protected?
  3. 
      
Loading...