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