----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14439/#review26648 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51911> Style: extra line here. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51918> The default (false) should also be specified in SyslogSourceConfigurationConstants flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/14439/#comment51919> Please mark this class as @InterfaceAudience.Private and @InterfaceStability.Evolving flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/14439/#comment51912> This should be SyslogSourceConfigurationConstants.DEFAULT_KEEP_FIELDS flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/14439/#comment51913> Style: Use camel-caps: keepTimeAndHost would be the right casing. However, you should just keep the names consistent and make it keepFields. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/14439/#comment51914> Style: Would be better to say this.keepFields = keepFields flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java <https://reviews.apache.org/r/14439/#comment51915> Style: The brace should stay on the same line as the conditional. If the line was too long to fit the brace (>80 chars), then keep the brace at the same indentation level as the "if". flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51925> Don't hard-code the port... multiple tests running at the same time (maybe multiple builds on the same Jenkins box) will fail. Start up the service on port 0 and get the kernel-selected port from the running service for use in the test. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51926> Style: Remove this extraneous line unless you want to document the return param (I think it's obvious). flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51924> Style: Come up with a better name than doTest. How about runKeepFieldsTest() ? Also, please add a comment here documenting what this test does. Something along the lines of: // Tests the keepFields configuration parameter (enabled or disabled) using SyslogTcpSource. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51920> Style: This comment should be indented. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51921> Don't catch this, just let testKeepFields throw IOException. It's a test and if it throws it will fail, which is fine. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51922> e.printStackTrace() is considered bad practice these days because it can easily go un-noticed. You should almost always LOG an exception like this or rethrow it. But in this case you can just remove the whole try/catch block and not worry about it. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java <https://reviews.apache.org/r/14439/#comment51923> Same thing here, remove the try/catch. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java <https://reviews.apache.org/r/14439/#comment51916> Style: Line continuations like this are usually indented 4 spaces instead of 2. flume-ng-doc/sphinx/FlumeUserGuide.rst <https://reviews.apache.org/r/14439/#comment51917> Style: match the indentation of this line to the word "Setting" above. Looks good overall, just a few stylistic suggestions for the most part. - Mike Percy On Oct. 2, 2013, 1:04 a.m., Jeff jlord wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14439/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2013, 1:04 a.m.) > > > Review request for Flume. > > > Repository: flume-git > > > Description > ------- > > Attaching a patch which introduces a boolean keepFields which defaults to > false. When set to true this will preserve the timestamp and hostname in the > body of the event. Additionally I have added a test for SyslogTcpSource > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java > 5a73c88 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > db9e0fd > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java > c2a29a1 > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java > 2d7a429 > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java > 7208464 > flume-ng-doc/sphinx/FlumeDeveloperGuide.rst 2be9c68 > flume-ng-doc/sphinx/FlumeUserGuide.rst dac3ce7 > > Diff: https://reviews.apache.org/r/14439/diff/ > > > Testing > ------- > > > Thanks, > > Jeff jlord > >
