You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@flume.apache.org by Jeff jlord <jl...@cloudera.com> on 2013/11/01 01:43:38 UTC

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

Review request for Flume 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

Re: Review Request 15163: FLUME-2217 Preserve priority, timestamp and hostname fields in MultiportSyslogTcp, SyslogTcp and Udp sources

Posted by Mike Percy <mp...@apache.org>.

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

Re: Review Request 15163: FLUME-2217 Preserve priority, timestamp and hostname fields in MultiportSyslogTcp, SyslogTcp and Udp sources

Posted by Mike Percy <mp...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/#review30140
-----------------------------------------------------------

Ship it!


+1, LGTM. Thanks for your persistence Jeff! Please attach the patch to the JIRA.

- Mike Percy


On Dec. 10, 2013, 12:23 a.m., Jeff jlord wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15163/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2013, 12:23 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 8687cb7 
> 
> 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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/
-----------------------------------------------------------

(Updated Dec. 10, 2013, 12:23 a.m.)


Review request for Flume, Hari Shreedharan and Mike Percy.


Changes
-------

New diff 


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 (updated)
-----

  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 8687cb7 

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/
-----------------------------------------------------------

(Updated Dec. 9, 2013, 11:53 p.m.)


Review request for Flume, Hari Shreedharan and Mike Percy.


Changes
-------

Added test back to syslogparser for keepFields=false which is the default behavior


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 (updated)
-----

  flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 884fd62 
  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/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/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/SyslogUDPSource.java 96a9e85 
  flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java f2ea932 
  flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java f2ea932 
  flume-ng-core/src/test/java/org/apache/flume/formatter/output/TestBucketPath.java c441c4a 
  flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 680e592 
  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/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/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/TestSyslogUdpSource.java eae26ed 
  flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 898096b 
  flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 898096b 
  flume-ng-doc/sphinx/FlumeUserGuide.rst 8687cb7 
  flume-ng-doc/sphinx/FlumeUserGuide.rst 8687cb7 

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/
-----------------------------------------------------------

(Updated Dec. 9, 2013, 11:04 p.m.)


Review request for Flume, Hari Shreedharan and Mike Percy.


Changes
-------

All changes implemented except for one as previously mentioned.
Thank you for the thorough review.


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 (updated)
-----

  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/formatter/output/TestBucketPath.java c441c4a 
  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 8687cb7 

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

Re: Review Request 15163: FLUME-2217 Preserve priority, timestamp and hostname fields in MultiportSyslogTcp, SyslogTcp and Udp sources

Posted by Mike Percy <mp...@apache.org>.

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

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

Changes
-------

Removed baosRaw and extra byte array.
Modified the initial parsing to include Priority e.g. <10>
Modified the regex to account for this and we should be good now.
Was able to build successfully.
Please let me know if there is anything else I can do to get this committed.

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 (updated)
-----

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/
-----------------------------------------------------------

(Updated Nov. 2, 2013, 9:51 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 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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15163/
-----------------------------------------------------------

(Updated Nov. 2, 2013, 9:47 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 (updated)
-----

  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.


File Attachments
----------------


  https://reviews.apache.org/media/uploaded/files/2013/11/02/FLUME-2217.2.patch


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

(Updated Nov. 2, 2013, 9:45 p.m.)

Review request for Flume, Hari Shreedharan and Mike Percy.

Changes
-------

Changes made.
If we split this into two jiras than it might make sense to have one be for the multiport source the other be for the tcp/udp sources.
There really wasn't a good logical separation as when I added the keepFields to the multiport it just happened that the priority was included there.
Not the case for tcp/udp. Which was the reason I wanted to go ahead and deprecate tcp source to make things easier to manage.

Repository: flume-git

Description
-------

Diffs
-----

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.

File Attachments (updated)
----------------

https://reviews.apache.org/media/uploaded/files/2013/11/02/FLUME-2217.2.patch

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

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