> 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 <[email protected]> > 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 > >
