----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4502/#review6429 -----------------------------------------------------------
Thanks for the patch Hari. The changes look good. Some minor feedback follows: * We follow the a variant of standard Java coding conventions. Please make sure your code is formatted accordingly. More details on this can be found at https://cwiki.apache.org/confluence/display/FLUME/How+to+Contribute#HowtoContribute-CodeQuality. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4502/#comment14024> This constant is defined twice - ComponentConfiguration and FlumeConfiguration. It will be better to externalize it in one constants class and reference it from there. Same for other constants too. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4502/#comment14023> please use different variable name as it is error prone to use names that shadow member variables. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4502/#comment14025> nit: suggest renaming this to failIfConfigured flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java <https://reviews.apache.org/r/4502/#comment14026> nit: suggest using a switch/case flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java <https://reviews.apache.org/r/4502/#comment14027> ("Cannot create configuration! Unknown Type specified: " + component) flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java <https://reviews.apache.org/r/4502/#comment14028> This should extend FlumeException to be consistent. flume-ng-configuration/src/main/java/org/apache/flume/conf/Context.java <https://reviews.apache.org/r/4502/#comment14022> Please do not change the package as that will break backward compatibility. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4502/#comment14021> These dependencies are missing. You need to move them into this module. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4502/#comment14029> This seems redundant as the addRawProperty() method will perform these checks anyway. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4502/#comment14032> I see that this check has been disabled for various components. Without this, the active component check, required attribute checks will not happen. How is the code handling those right now? flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4502/#comment14030> Looks like this check needs to happen before the if(channelSet.size() == 0) check. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4502/#comment14034> Need to handle the case where the config is not specified, but the component is a standard component. In such cases, the component type enum should be used to determine the config holder class. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java <https://reviews.apache.org/r/4502/#comment14035> Please do not use exclamation marks in error messages. flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/4502/#comment14036> s/Configuring channels - Arvind On 2012-03-27 06:17:30, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4502/ > ----------------------------------------------------------- > > (Updated 2012-03-27 06:17:30) > > > Review request for Flume. > > > Summary > ------- > > Main config component. > > > This addresses bug FLUME-1052. > https://issues.apache.org/jira/browse/FLUME-1052 > > > Diffs > ----- > > flume-ng-configuration/pom.xml PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java > PRE-CREATION > flume-ng-configuration/src/main/java/org/apache/flume/conf/Context.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationError.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java > PRE-CREATION > flume-ng-core/pom.xml 37fb112 > flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 > > flume-ng-core/src/main/java/org/apache/flume/conf/ConfigurableComponent.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/conf/Configurables.java > 84492e5 > > flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java > 3358cf4 > flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java > 92ff6fe > flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java > 7e26e2a > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/FlumeConfiguration.java > d66f6d1 > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java > 50b9f0c > pom.xml c91222f > > Diff: https://reviews.apache.org/r/4502/diff > > > Testing > ------- > > Functional testing done. > > > Thanks, > > Hari > >
