You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2014/08/02 10:11:37 UTC
Review Request 24202: FLUME-2438 Make Syslog source message body configurable
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-2438
https://issues.apache.org/jira/browse/FLUME-2438
Repository: flume-git
Description
-------
commit 2148aa92e576f0f3e47ffd5824d5d40b3fde7bf8
Author: Abraham Elmahrek <ab...@elmahrek.com>
Date: Fri Aug 1 18:33:11 2014 -0700
FLUME-2438 Make Syslog source message body configurable
keepFields in the agent configuration can accept a space
separated list of fields that include either "timestamp"
or "hostname" or both. This will tell the agent to include
the specified fields in the message body.
:100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
:100644 100644 557d121... 4bc9971... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
:100644 100644 985949c... 6c9c207... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
:100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
:100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
:100644 100644 a77bfc9... e1f1ef8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
:100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
:100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
:100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
:100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
:100644 100644 daf6e72... 1699fb1... M flume-ng-doc/sphinx/FlumeUserGuide.rst
:000000 100644 0000000... 99cf309... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
Diff: https://reviews.apache.org/r/24202/diff/
Testing
-------
Added integration test and updated unit tests
Thanks,
Abraham Elmahrek
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java, line 104
> > <https://reviews.apache.org/r/24202/diff/3/?file=651390#file651390line104>
> >
> > We should use a SyslogUtils constant for this key, like the ones below
>
> Abraham Elmahrek wrote:
> I'm +1 to this in general, but there appears to be exceptions in code. See SyslogUtils Line #16 in this review. The "host" and "timestamp" headers are simply added as string literals.
That's true, but it's prob better to avoid duplicating string literals all over the place.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review49790
-----------------------------------------------------------
On Aug. 12, 2014, 1:52 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 1:52 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java, line 146
> > <https://reviews.apache.org/r/24202/diff/3/?file=651389#file651389line146>
> >
> > I know this makes your life a little harder but let's also support "all" and "none" as first class citizens and "true" and "false" as deprecated fallbacks. Otherwise it is not intuitively named
Indeed.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java, line 104
> > <https://reviews.apache.org/r/24202/diff/3/?file=651390#file651390line104>
> >
> > We should use a SyslogUtils constant for this key, like the ones below
I'm +1 to this in general, but there appears to be exceptions in code. See SyslogUtils Line #16 in this review. The "host" and "timestamp" headers are simply added as string literals.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java, line 185
> > <https://reviews.apache.org/r/24202/diff/3/?file=651390#file651390line185>
> >
> > Maybe there is no good way to share this... but just wanted to throw it out there that if it's possible it would be cool to share this code between the sources.
> >
> > If it's not gonna be a real net benefit to the code, then cest la vie, just thought I'd throw the idea out there.
Excellent idea. Adding to SyslogUtils.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java, line 120
> > <https://reviews.apache.org/r/24202/diff/3/?file=651394#file651394line120>
> >
> > Hmm, can we avoid plumbing this keepAllFields thing everywhere and just recalculate it once at the top of any function that needs it? Just add a helper function shouldKeepAllFields(String[] keepFields) or something like that. Ideally we could share that helper function between both sources, so it could live in some util class.
Excellent idea. Adding to SyslogUtils.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java, line 158
> > <https://reviews.apache.org/r/24202/diff/3/?file=651401#file651401line158>
> >
> > It looks like there is a lot of duplicated code between these two integration tests. Can it be shared via some utility library?
Pulled out into a utility class.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java, line 58
> > <https://reviews.apache.org/r/24202/diff/3/?file=651398#file651398line58>
> >
> > Why did you add a newline only to this one?
It seems there isn't always a newline attached to the body. Body will exclude trailing new line.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java, line 73
> > <https://reviews.apache.org/r/24202/diff/3/?file=651395#file651395line73>
> >
> > How about private final static String[] KEEP_FIELDS_NONE = { "none" };
Removed.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java, line 99
> > <https://reviews.apache.org/r/24202/diff/3/?file=651396#file651396line99>
> >
> > See, this is kinda weird, having both fields in these tests, you should be able to just infer the value of all from the array
Removed in favor of persisting the arg string.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java, line 143
> > <https://reviews.apache.org/r/24202/diff/3/?file=651389#file651389line143>
> >
> > New comment doesn't really make sense
Removed it.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review49790
-----------------------------------------------------------
On Aug. 5, 2014, 12:10 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 12:10 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 6, 2014, 8:18 p.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java, line 58
> > <https://reviews.apache.org/r/24202/diff/3/?file=651398#file651398line58>
> >
> > Why did you add a newline only to this one?
>
> Abraham Elmahrek wrote:
> It seems there isn't always a newline attached to the body. Body will exclude trailing new line.
Actually, my mistake, I've simply copied from TestSyslogTcpSource which includes the new line. In the case of UDP, it doesn't seem to require a trailing new line. Thank you for pointing this out.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review49790
-----------------------------------------------------------
On Aug. 5, 2014, 12:10 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 12:10 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review49790
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87127>
New comment doesn't really make sense
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87128>
I know this makes your life a little harder but let's also support "all" and "none" as first class citizens and "true" and "false" as deprecated fallbacks. Otherwise it is not intuitively named
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
<https://reviews.apache.org/r/24202/#comment87129>
We should use a SyslogUtils constant for this key, like the ones below
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
<https://reviews.apache.org/r/24202/#comment87130>
Nit: mind fixing the indentation on these case comments, they are kinda weird.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
<https://reviews.apache.org/r/24202/#comment87131>
And this one
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
<https://reviews.apache.org/r/24202/#comment87145>
Maybe there is no good way to share this... but just wanted to throw it out there that if it's possible it would be cool to share this code between the sources.
If it's not gonna be a real net benefit to the code, then cest la vie, just thought I'd throw the idea out there.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87132>
Hmm, can we avoid plumbing this keepAllFields thing everywhere and just recalculate it once at the top of any function that needs it? Just add a helper function shouldKeepAllFields(String[] keepFields) or something like that. Ideally we could share that helper function between both sources, so it could live in some util class.
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87133>
How about private final static String[] KEEP_FIELDS_NONE = { "none" };
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
<https://reviews.apache.org/r/24202/#comment87134>
See, this is kinda weird, having both fields in these tests, you should be able to just infer the value of all from the array
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
<https://reviews.apache.org/r/24202/#comment87140>
Hmm. Do we have an "all" test here?
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
<https://reviews.apache.org/r/24202/#comment87135>
Why did you add a newline only to this one?
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
<https://reviews.apache.org/r/24202/#comment87136>
Would be great to continue testing both true and all
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
<https://reviews.apache.org/r/24202/#comment87138>
Test both false and none:
runKeepFieldsTest("none");
// For backwards compatibility.
runKeepFieldsTest("false");
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/24202/#comment87125>
Since this is now a list, can we retire "true" and "false" while keeping them as aliases, and make the new default "none" and add "all" as the equivalent of "true"? We can also mention "true" and "false" are deprecated but currently supported for backwards compat. We can remove them in some future release, say Flume 1.7.
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87124>
On my box, these tests took way too long to run. Can we reduce this to a handful of seconds?
Compare the running time to the other integration tests:
Running org.apache.flume.test.agent.TestRpcClientCommunicationFailure
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.962 sec
Running org.apache.flume.test.agent.TestMultiportSyslogTCPSource
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 144.646 sec
Running org.apache.flume.test.agent.TestFileChannel
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 16.581 sec
Running org.apache.flume.test.agent.TestRpcClient
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.979 sec
Running org.apache.flume.test.agent.TestSyslogTCPSource
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 144.647 sec
Running org.apache.flume.test.agent.TestSpooldirSource
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 16.917 sec
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87118>
Nit: memory channel might give a faster test time
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87126>
You don't need the Hadoop jar for these tests, and actually I just removed this functionality when I committed FLUME-1920 so you can just remove calls to this and the setAgentClasspath thing.
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87117>
It looks like there is a lot of duplicated code between these two integration tests. Can it be shared via some utility library?
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87119>
Nit: Sinks
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87120>
Is 10 seconds really needed? Is there some other way we can do this, maybe by waiting for the process to die via StagedInstall? This really slows down the test times
- Mike Percy
On Aug. 5, 2014, 12:10 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 12:10 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 14, 2014, 8:41 p.m., Mike Percy wrote:
> > I think this is good to go.
Thanks Mike!
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50633
-----------------------------------------------------------
On Aug. 14, 2014, 8:08 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 8:08 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50633
-----------------------------------------------------------
Ship it!
I think this is good to go.
- Mike Percy
On Aug. 14, 2014, 8:08 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 8:08 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/
-----------------------------------------------------------
(Updated Aug. 14, 2014, 8:08 p.m.)
Review request for Flume.
Bugs: FLUME-2438
https://issues.apache.org/jira/browse/FLUME-2438
Repository: flume-git
Description
-------
commit 321a62abc29ce495743b174cb8dec648e5448e2f
Author: Abraham Elmahrek <ab...@elmahrek.com>
Date: Fri Aug 1 18:33:11 2014 -0700
FLUME-2438 Make Syslog source message body configurable
keepFields in the agent configuration can accept a space
separated list of fields that include either "timestamp"
or "hostname" or both. This will tell the agent to include
the specified fields in the message body.
:100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
:100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
:100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
:100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
:100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
:100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
:100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
:100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
:100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
:100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
:100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
:100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
:000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
:000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
Diff: https://reviews.apache.org/r/24202/diff/
Testing
-------
Added integration test and updated unit tests
Thanks,
Abraham Elmahrek
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50612
-----------------------------------------------------------
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java
<https://reviews.apache.org/r/24202/#comment88479>
How about do something like this to parameterize the JUnit test: http://www.tutorialspoint.com/junit/junit_parameterized_test.htm
- Mike Percy
On Aug. 12, 2014, 1:52 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 1:52 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/
-----------------------------------------------------------
(Updated Aug. 12, 2014, 1:52 a.m.)
Review request for Flume.
Bugs: FLUME-2438
https://issues.apache.org/jira/browse/FLUME-2438
Repository: flume-git
Description
-------
commit 321a62abc29ce495743b174cb8dec648e5448e2f
Author: Abraham Elmahrek <ab...@elmahrek.com>
Date: Fri Aug 1 18:33:11 2014 -0700
FLUME-2438 Make Syslog source message body configurable
keepFields in the agent configuration can accept a space
separated list of fields that include either "timestamp"
or "hostname" or both. This will tell the agent to include
the specified fields in the message body.
:100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
:100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
:100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
:100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
:100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
:100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
:100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
:100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
:100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
:100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
:100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
:100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
:000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
:000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
Diff: https://reviews.apache.org/r/24202/diff/
Testing
-------
Added integration test and updated unit tests
Thanks,
Abraham Elmahrek
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 11, 2014, 7:44 p.m., Mike Percy wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java, line 96
> > <https://reviews.apache.org/r/24202/diff/4/?file=656299#file656299line96>
> >
> > Seems to me like this should be an array of Strings or a Set<String> to avoid splitting and indexOf()ing this over and over. Set or Map would probably be ideal to get O(1) lookup times, depending on how you want to write the loop.
Sets are cool.
> On Aug. 11, 2014, 7:44 p.m., Mike Percy wrote:
> > flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java, line 216
> > <https://reviews.apache.org/r/24202/diff/4/?file=656309#file656309line216>
> >
> > Did you mean for this to be part of the public API of this class? Seems like a private API to me.
Intended to be public, but can make private.
> On Aug. 11, 2014, 7:44 p.m., Mike Percy wrote:
> > flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java, line 22
> > <https://reviews.apache.org/r/24202/diff/4/?file=656309#file656309line22>
> >
> > This class needs documentation.
Could you be more specific? I'll add documentation at the class level for sure.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50204
-----------------------------------------------------------
On Aug. 8, 2014, 12:32 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 12:32 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
> On Aug. 11, 2014, 7:44 p.m., Mike Percy wrote:
> > flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java, line 22
> > <https://reviews.apache.org/r/24202/diff/4/?file=656309#file656309line22>
> >
> > This class needs documentation.
>
> Abraham Elmahrek wrote:
> Could you be more specific? I'll add documentation at the class level for sure.
Basically it goes back to the public vs. private thing... seemed weird to me that we would include a full test in the SyslogAgent and also provide public methods that no one outside SyslogAgent was calling. Seems to violate SRP. Since this is a test class, it is less critical, but it's also an infrastructure class, so worth thinking about.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50204
-----------------------------------------------------------
On Aug. 12, 2014, 1:52 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 1:52 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 11, 2014, 7:44 p.m., Mike Percy wrote:
> > flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java, line 22
> > <https://reviews.apache.org/r/24202/diff/4/?file=656309#file656309line22>
> >
> > This class needs documentation.
>
> Abraham Elmahrek wrote:
> Could you be more specific? I'll add documentation at the class level for sure.
>
> Mike Percy wrote:
> Basically it goes back to the public vs. private thing... seemed weird to me that we would include a full test in the SyslogAgent and also provide public methods that no one outside SyslogAgent was calling. Seems to violate SRP. Since this is a test class, it is less critical, but it's also an infrastructure class, so worth thinking about.
Understood. In order to make this more complete I'll have to parameterize SyslogAgent and move the test logic to the TestSyslogSource class.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50204
-----------------------------------------------------------
On Aug. 12, 2014, 1:52 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2014, 1:52 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/#review50204
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87815>
Seems to me like this should be an array of Strings or a Set<String> to avoid splitting and indexOf()ing this over and over. Set or Map would probably be ideal to get O(1) lookup times, depending on how you want to write the loop.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87816>
I don't see any reason we should be calling this expensive function for every event. This can happen once at Source configure() time.
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87831>
public static void if possible, and same with the other version of this function please
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87832>
Needs some kind of doc. Just indicate that this tests when keepFields is "none".
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
<https://reviews.apache.org/r/24202/#comment87833>
Why null instead of "none" or some explicit representation of none?
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
<https://reviews.apache.org/r/24202/#comment87851>
This integration test and the other have a 1-line difference, otherwise they are copy/paste. Just make them one test class and loop over the enum.
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87838>
Missing Apache license header (fails RAT check when you do mvn clean install -DskipTests from the top level)
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87846>
This class needs documentation.
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87834>
unused?
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87844>
Why DataOutputStream? How about BufferedOutputStream?
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87848>
How about just setup() or configure()
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87849>
How about just start()? Needs docs. Indicate that it blocks until it starts.
Also, why is keepFields part of this function, why not make it part of setup/configure?
Also, it doesn't like you actually use the attempts or timeout parameter. Better to reduce the surface area of the API and just have one start function. YAGNI.
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87836>
Copy/paste? Doesn't use Hadoop stuff
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87850>
Would be great if the error message included how much time it waited or number of attempts that were made.
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87855>
Needs docs
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87852>
... " after X attempts over Y ms"
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87853>
AFAICT we never call this with anything but the default params. Might as well use those params directly or at least make the multi-param version of the runKeepFieldsTest() method private.
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java
<https://reviews.apache.org/r/24202/#comment87854>
Did you mean for this to be part of the public API of this class? Seems like a private API to me.
- Mike Percy
On Aug. 8, 2014, 12:32 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24202/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2014, 12:32 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2438
> https://issues.apache.org/jira/browse/FLUME-2438
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> commit 321a62abc29ce495743b174cb8dec648e5448e2f
> Author: Abraham Elmahrek <ab...@elmahrek.com>
> Date: Fri Aug 1 18:33:11 2014 -0700
>
> FLUME-2438 Make Syslog source message body configurable
>
> keepFields in the agent configuration can accept a space
> separated list of fields that include either "timestamp"
> or "hostname" or both. This will tell the agent to include
> the specified fields in the message body.
>
> :100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
> :100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
> :100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
> :100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
> :100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
> :100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
> :100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
> :100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
> :100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
> :100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
> :100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
> :100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
> :000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
> :000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
> flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
> flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
> flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
> flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24202/diff/
>
>
> Testing
> -------
>
> Added integration test and updated unit tests
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/
-----------------------------------------------------------
(Updated Aug. 8, 2014, 12:32 a.m.)
Review request for Flume.
Bugs: FLUME-2438
https://issues.apache.org/jira/browse/FLUME-2438
Repository: flume-git
Description
-------
commit 321a62abc29ce495743b174cb8dec648e5448e2f
Author: Abraham Elmahrek <ab...@elmahrek.com>
Date: Fri Aug 1 18:33:11 2014 -0700
FLUME-2438 Make Syslog source message body configurable
keepFields in the agent configuration can accept a space
separated list of fields that include either "timestamp"
or "hostname" or both. This will tell the agent to include
the specified fields in the message body.
:100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
:100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
:100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
:100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
:100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
:100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
:100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
:100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
:100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
:100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
:100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
:100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
:000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
:000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java a6bd5e9
flume-ng-tests/src/test/java/org/apache/flume/test/util/SyslogAgent.java PRE-CREATION
Diff: https://reviews.apache.org/r/24202/diff/
Testing
-------
Added integration test and updated unit tests
Thanks,
Abraham Elmahrek
Re: Review Request 24202: FLUME-2438 Make Syslog source message body
configurable
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24202/
-----------------------------------------------------------
(Updated Aug. 5, 2014, 12:10 a.m.)
Review request for Flume.
Changes
-------
Including priority and version. Also adding a test for multiport syslog tcp source.
Bugs: FLUME-2438
https://issues.apache.org/jira/browse/FLUME-2438
Repository: flume-git
Description (updated)
-------
commit 321a62abc29ce495743b174cb8dec648e5448e2f
Author: Abraham Elmahrek <ab...@elmahrek.com>
Date: Fri Aug 1 18:33:11 2014 -0700
FLUME-2438 Make Syslog source message body configurable
keepFields in the agent configuration can accept a space
separated list of fields that include either "timestamp"
or "hostname" or both. This will tell the agent to include
the specified fields in the message body.
:100644 100644 427e0e3... 643bd67... M flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java
:100644 100644 557d121... 996b31b... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java
:100644 100644 985949c... a312cb6... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
:100644 100644 e84e4b6... 03e593c... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
:100644 100644 01b8905... 40a96a8... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
:100644 100644 a77bfc9... e966856... M flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
:100644 100644 9b97c8c... 72dc5c1... M flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java
:100644 100644 2809163... 594e456... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java
:100644 100644 22fa200... d32e9cc... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java
:100644 100644 95ee48c... 47f49d5... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
:100644 100644 82b7dd0... 5a4db9d... M flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
:100644 100644 daf6e72... d179bf8... M flume-ng-doc/sphinx/FlumeUserGuide.rst
:000000 100644 0000000... dc463d5... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java
:000000 100644 0000000... 8e50386... A flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/MultiportSyslogTCPSource.java 427e0e3
flume-ng-core/src/main/java/org/apache/flume/source/SyslogParser.java 557d121
flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java 985949c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java e84e4b6
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 01b8905
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java a77bfc9
flume-ng-core/src/test/java/org/apache/flume/source/TestMultiportSyslogTCPSource.java 9b97c8c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogParser.java 2809163
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 22fa200
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java 95ee48c
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java 82b7dd0
flume-ng-doc/sphinx/FlumeUserGuide.rst daf6e72
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestMultiportSyslogTCPSource.java PRE-CREATION
flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestSyslogTCPSource.java PRE-CREATION
Diff: https://reviews.apache.org/r/24202/diff/
Testing
-------
Added integration test and updated unit tests
Thanks,
Abraham Elmahrek