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