----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15163/#review28052 -----------------------------------------------------------
A few style nits and suggestions below, other than that at a high level I think you should split this into two patches so it's easy to review: One should make the keepFields thing keep the priority (this is basically a bug-fix), the 2nd patch should extend this functionality to Multiport and UDP as you are also doing here. flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/15163/#comment54568> style: we always use braces on an else flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment54574> baosRaw is not a descriptive name. Also, this is not a ByteArrayOutputStream. If this variable really is required, consider using a variable name like rawSyslogLine or something like that. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment54569> style: if your "if" statement has a line-break always use curly braces Also, this would be easier to read if you did it like: if (!keepFields) { if (msgBody != null) && msgBody.length() > 0) { body = msgBody.getBytes(); } else { // Parse failed. body = baos.toByteArray(); } } else { body = baosRaw; // Change this variable name though } flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment54575> Is this baosRaw thing required? Could we somehow just use baos instead? I am not sure, just an idea. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java <https://reviews.apache.org/r/15163/#comment54572> Doesn't look like this import is used. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment54570> style: Don't reorder these includes, it's just noisy and my editor might not agree with yours so nobody is "right". Just keep with the existing pattern in the file. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment54571> style: indent this 4 spaces - Mike Percy On Nov. 1, 2013, 2:20 p.m., Jeff jlord wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15163/ > ----------------------------------------------------------- > > (Updated Nov. 1, 2013, 2:20 p.m.) > > > Review request for Flume, Hari Shreedharan and Mike Percy. > > > Repository: flume-git > > > Description > ------- > > Here is a first pass at this. > Please not that the functionality of all 3 sources was modified slightly such > that with this patch we will now preserve the syslog priority as well as the > timestamp and header. e.g. > <10>2013-10-31T17:36:27.381-07:00 localhost.localdomain test UDP syslog data > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java > 884fd62 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java > bf3305c > flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > 7a12d27 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java > 96a9e85 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java > f2ea932 > > flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java > 680e592 > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java > 258c2f1 > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java > a6a1d5b > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java > eae26ed > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java > 898096b > flume-ng-doc/sphinx/FlumeUserGuide.rst e38bb67 > > Diff: https://reviews.apache.org/r/15163/diff/ > > > Testing > ------- > > Added a brand new TestSyslogUdp Class to verify functionality. > Updated Syslog parser test to verify as well. > > > Thanks, > > Jeff jlord > >
