----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3924/#review5213 -----------------------------------------------------------
Hari - thanks for the patch. Some feedback follows: * Please do not use tabs anywhere. Instead, please use two spaces for indent where you are otherwise using a tab. * Please remove any trailing white spaces from the sources. These are highlighted with red color in the review board diff view. * Since this is one of the many clients that we would like to build, I suggest that you move it under a top-level module called flume-ng-clients. That way the root directory will not end up containing a lot of client modules. branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java <https://reviews.apache.org/r/3924/#comment11406> This initialization should ideally happen in the overridden activateOptions() method of the appender. Also, as pointed out by Ralph, the configuration will need to be injected via setter methods, so please add them. One thing that I am not too sure about is how the NettyTransceiver is supposed to be used in a multi-threaded environment. It will be good to find that out before making the assumption that it is safe for such use. branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java <https://reviews.apache.org/r/3924/#comment11405> Since this appender is part of Flume distribution, the header names should be in flume's private namespace. For example - LOGGER_NAME should be "flume.client.log4j.logger.name". You can do this by having providing these standard names as part of the enum state. Also, it is customary to have the first enum be OTHER which ensures that the ordinal for unknown headers will always remain 0. - Arvind On 2012-02-16 18:50:02, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3924/ > ----------------------------------------------------------- > > (Updated 2012-02-16 18:50:02) > > > Review request for Flume. > > > Summary > ------- > > A class which takes in a LoggingEvent and sends it to a predefined AvroSource > at a given host and port. > > > This addresses bug FLUME-886. > https://issues.apache.org/jira/browse/FLUME-886 > > > Diffs > ----- > > branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION > > branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java > PRE-CREATION > > branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java > PRE-CREATION > > branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java > PRE-CREATION > > branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties > PRE-CREATION > branches/flume-728/pom.xml 1244858 > > Diff: https://reviews.apache.org/r/3924/diff > > > Testing > ------- > > Added unit tests. > > > Thanks, > > Hari > >
