Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume >> mail # dev >> Review Request 14439: Syslog source strips timestamp and hostname from log message body


Copy link to this message
-
Re: Review Request 14439: Syslog source strips timestamp and hostname from log message body

-----------------------------------------------------------
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: