You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Mike Percy <mp...@apache.org> on 2013/12/04 04:39:39 UTC
Re: Review Request 15163: FLUME-2217 Preserve priority,
timestamp and hostname fields in MultiportSyslogTcp,
SyslogTcp and Udp sources
-----------------------------------------------------------
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
>
>
Re: Review Request 15163: FLUME-2217 Preserve priority,
timestamp and hostname fields in MultiportSyslogTcp,
SyslogTcp and Udp sources
Posted by Jeff jlord <jl...@cloudera.com>.
> On Dec. 4, 2013, 3:39 a.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java, line 80
> > <https://reviews.apache.org/r/15163/diff/3/?file=378701#file378701line80>
> >
> > This is actually a really good test. Can you please loop twice, once with keepFields = true, another with keepFields = false?
In order to loop once again with keepFields = false I will need to rebuild the messagebody in a different way that breaks out the priority for each message. Such that we can compare the messagebody sans Priority to the event body.
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/#review29691
-----------------------------------------------------------
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
>
>