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/#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.apach