Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume >> mail # dev >> Review Request 24202: FLUME-2438 Make Syslog source message body configurable


Copy link to this message
-
Re: 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/#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: