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

Switch to Threaded View
Flume >> mail # dev >> Review Request 14176: Add support for IP filtering on AvroSource


Copy link to this message
-
Re: Review Request 14176: Add support for IP filtering on AvroSource

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14176/#review26237
-----------------------------------------------------------
Thanks for the patch, Ted! Looks pretty good. I have some comments below, especially about the configuration.
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51262>

    Can we make this ipFilterRules? All new config params are being camel cased.

flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51263>

    You need a check here to ensure that patternRuleConfigDefinition is not null. If it is, then the config is invalid

flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51264>

    All of these can be final.

flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51265>

    Perhaps it is time to use a builder here?

flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51266>

    Nit: patternRuleConfigDefinition.isEmpty() == false can be written as
    !patternRuleConfigDefinition.isEmpty()
    
    This check should happen in the configure method, since having ip filter enabled with no rule is a misconfiguration and should fail the component - else the result would be that the component behaves in an unexpected way

flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51268>

    I understand that this is being done so we can have multiple rules, but I'd prefer the rules to be made more explicit. A typo in the config (a vs d) should not cause issues which turns the rules upside down. I think the rule format should be more like:
    
    allow/deny:ip/name:pattern.
    
    Agreed that it makes the rules more verbose, but since it is compiled only once initially, I don't see a performance issue either.

flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java
<https://reviews.apache.org/r/14176/#comment51269>

    Could you add a test where a deny is happening where the accepted random host address is a random IP address. Something like:
    
    a:i:45.3.2.4 - this should be denied since the connection is from local host (of course you must also ensure that that random IP is not the local host - maybe generate using random method, not hardcode)

flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/14176/#comment51270>

    Trailing whitespace

flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/14176/#comment51271>

    trailing white space. Also please do not mention details about the underlying implementation - we could change that later without changing the parameters. So the user should not depend on the implementation.
- Hari Shreedharan
On Sept. 17, 2013, 7:58 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14176/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2013, 7:58 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: Flume-2189
>     https://issues.apache.org/jira/browse/Flume-2189
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> This patch does the following
> adds support for ipFiltering on the avroSource
> adds unit testing for configuring ipFiltering on the avroSource
> adds documentation in the user's guide
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java f23cd93
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 2667a6f
>   flume-ng-doc/sphinx/FlumeUserGuide.rst c614991
>   pom.xml 25ea4e7
>