----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3243/#review4707 -----------------------------------------------------------
Thanks for the patch Jeff, andy apologies for the delayed response. Some general comments about the change: 1. Please use two-space indent and no tabs in your code. Also, please remove trailing whitespaces wherever they are present. Reviewboard highlights them with red color. 2. Instead of adding it to the core module, I suggest creating an explicit client module that contains this as one of the submodules. The reason is that we do not want an application that wishes to use the log4j appender for flume to end up adding all of its dependencies in the classpath. Without an explicitly packaged system, it is not clear which dependencies are required and which are not. 3. The current implementation is not capturing any logging event metadata at all. Looking at the LoggingEvent API from log4j 2.1.16, I feel that at least the logger name, the timestamp and the logging level must be captured as flume event headers. 4. The event body is currently turned into a byte payload using the default encoding. However, I suggest using UTF-8 encoding as a standard and also adding a header in the event which identifies the character encoding used for extracting the bytes. 5. What is the performance impact of requesting a specific client on every logging request? Would it make sense to have the client initialized once and used for repeated append calls? I am not sure of which is better, and suggest you looking into the cost of doing either approach in order to choose the final implementation. Thanks, Arvind - Arvind On 2011-12-16 22:25:40, Jeff de Anda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3243/ > ----------------------------------------------------------- > > (Updated 2011-12-16 22:25:40) > > > Review request for Flume and Eric Sammer. > > > Summary > ------- > > Created log4j appender using avro source. > > > This addresses bug flume-886. > https://issues.apache.org/jira/browse/flume-886 > > > Diffs > ----- > > ./flume-ng-core/src/main/java/org/apache/flume/log4j/AvroAppender.java > PRE-CREATION > ./flume-ng-core/src/test/java/org/apache/flume/log4j/TestAvroAppender.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3243/diff > > > Testing > ------- > > > Thanks, > > Jeff > >
