----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15163/#review29691 -----------------------------------------------------------
Just a pile of stylistic comments flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57131> Nit: Let's indent this to the same level as the surrounding code flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57129> What's with these empty strings? Please either remove or comment these. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57130> This means 2 digits, right? We should really keep this next to the pattern it's affecting above flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57139> Nit: the "stamp" comment applies to the whole block. I don't think it's needed, it's just confusing down here on the last line of the block. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/15163/#comment57132> Nit: removed a space unnecessarily flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57133> Style nit: Leave spaces around the "+" like: priority + goodData1.trim() flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57134> Style: Same here flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57135> Style: Same here flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57136> Style: Same here flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57137> Style: Same here flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java <https://reviews.apache.org/r/15163/#comment57157> Make this a regular boolean primitive. You can do that because you fall back to a default value in the configuration parsing logic below. flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java <https://reviews.apache.org/r/15163/#comment57158> Make this a boolean primitive flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java <https://reviews.apache.org/r/15163/#comment57159> boolean primitive (simplifies error handling later, you don't have to check for null when unboxing) flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java <https://reviews.apache.org/r/15163/#comment57140> Please add a brief comment for @param decoder so the Javadoc stays correct. flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/15163/#comment57142> Let's add the following annotations to this class, since we are breaking API backwards compatibility and we should be clear that this is not a public API: @InterfaceAudience.Private @InterfaceStability.Evolving flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/15163/#comment57160> Make this a boolean primitive flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/15163/#comment57161> If this is not a primitive (Boolean object) and somehow it's set to null, then this line will throw a NullPointerException since Java implicitly calls keepFields.booleanValue() when this is unboxed. That's why I suggest using a primitive when possible. flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java <https://reviews.apache.org/r/15163/#comment57156> Style: keep this on the same line as the closing if bracket, i.e.: } else { flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/15163/#comment57162> Use a primitive boolean flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57152> I know you didn't write this, but please indent these comments to match the indentation of the surrounding code (one more level) flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57153> Just add this at the top of the case statement, bring it outside of the "if" statement (don't repeat yourself) flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/15163/#comment57154> Same as above flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java <https://reviews.apache.org/r/15163/#comment57163> Couple suggestions: 1. Use a primitive boolean 2. Since this is a constant, define it like this: private final static boolean DONT_KEEP_FIELDS = false; flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java <https://reviews.apache.org/r/15163/#comment57164> Use a boolean primitive flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java <https://reviews.apache.org/r/15163/#comment57165> This is actually a really good test. Can you please loop twice, once with keepFields = true, another with keepFields = false? flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/15163/#comment57166> Style: Indent this line 4 spaces and keep spaces around the + flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/15163/#comment57170> This function seems pointless when the raw event is a constant. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/15163/#comment57167> Hrm, how about just return bodyWithTandH.getBytes() if you prefer to keep this function flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57169> Style: extra line flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57168> Style: extra line flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57172> Why use this function at all? Why not just call bodyWithTandH.getBytes() ... seems clearer to me flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57173> Style nit: leave a space above this method flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57177> Better to Assert.assertNotNull(e) flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java <https://reviews.apache.org/r/15163/#comment57178> Nit: Extra space before parens flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/15163/#comment57179> Please add a comment that the minimum size (which is 10) overrides the 5 specified here. - Mike Percy On Nov. 6, 2013, 1:49 a.m., Jeff jlord wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15163/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2013, 1:49 a.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 3a3038c > > 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 > >
