----------------------------------------------------------- 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 > > Diff: https://reviews.apache.org/r/14176/diff/ > > > Testing > ------- > > > Thanks, > > Ted Malaska > >
