----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4809/#review7200 -----------------------------------------------------------
Thanks for the patch Prasad. I have a few nits/requests below, along with a refactoring request noted below. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/4809/#comment15902> While you are incorporating the feedback, it will be great if you could change this class name to be convention compliant (SyslogTcpHandler instead of syslogTcpHandler). flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/4809/#comment15901> Instead of doing this, please add a constructor that takes these flags as arguments and sets it up on a newly created syslogUtils object. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/4809/#comment15903> instead it will be something like: handler = new SyslogTcpHandler(eventSiz,e hasVersion, useRFC5424); flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/4809/#comment15898> typo: Boolean instead of boolean flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/4809/#comment15904> Class name SyslogUdpHandler please. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/4809/#comment15896> Prefer to use the constructor that has event size, udp flag and rfc5424 flag. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/4809/#comment15905> Suggest removing this constructor and the other one that takes the (isUdp, useRFC5424) arguments. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/4809/#comment15897> Suggest not modifying the constructor argument list for consistency. Instead have the caller use the setter method as is being used for hasVersion flag. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/4809/#comment15907> This adhoc parsing has become significantly complex and hard to follow. It will be better to instead have a standard solution that parses the event body into subcomponents as necessary. For example it could be done by: * using a table driven parser based on standard grammar * using an externally specified format to parse the body of the event * using a externally specified regular expression that parses the body of the event. Minimally, I request that this part be refactored so that the parsing of the body is done in the buildEvent() method by a secondary routine instead. This will ensure that the current implementation continues to work and the secondary parsing can be done defensively and thus default to old semantics if it is unable to parse the expected fields correctly. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/4809/#comment15900> Please remove this setter and instead force the use of the constructor instead. - Arvind On 2012-04-20 03:19:50, Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4809/ > ----------------------------------------------------------- > > (Updated 2012-04-20 03:19:50) > > > Review request for Flume, Arvind Prabhakar and Mike Percy. > > > Summary > ------- > > Support for timestamp and hostname parsing. > > > This addresses bug FLUME-1126. > https://issues.apache.org/jira/browse/FLUME-1126 > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > b0485b1 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java > 732cce5 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java > 653f5eb > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java > 3a7c486 > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java > 8b1f7c5 > > Diff: https://reviews.apache.org/r/4809/diff > > > Testing > ------- > > Updated syslog tests to cover timestamp and hostname handling. > Manually tested using syslog4j > > > Thanks, > > Prasad > >
