----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4115/#review5783 -----------------------------------------------------------
Thanks for the interim patch Hari. I did a quick walk through of these changes and have some feedback that follows. Some general issues: * The component configuration stubs do not report any configuration errors at this stage. While we cannot expect these stubs to do runtime validation, we should at least check all the static rules that may be being violated. For example a non-numeric port value is something that does not get caught/reported by the current implementation. * As implemented, the configuration validation mechanism only reports errors. It should also report warnings where default values override the specified values etc. More specific feedback below: flume-ng-configuration/pom.xml <https://reviews.apache.org/r/4115/#comment12598> Better to call it Flume NG Configuration only. flume-ng-configuration/pom.xml <https://reviews.apache.org/r/4115/#comment12600> Duplicate flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4115/#comment12623> should be private flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4115/#comment12624> Does not look like its being used, in which case it should be removed considering the risk it introduces. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java <https://reviews.apache.org/r/4115/#comment12625> This method is not returning any configuration values. Assuming it is work in progress - you should have a way by which the specialized component classes can use this without having to copy/rewrite this method. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java <https://reviews.apache.org/r/4115/#comment12614> Please make a ComponentTypeEnum that has SOURCE, CHANNEL, SINKPROCESSOR, CHANNELSELECTOR etc in it. flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java <https://reviews.apache.org/r/4115/#comment12615> Shouldn't this be Class<SinkConfiguration> ? Ideally, you want to have an enum that can return the actual class instance directly rather than doing a Class.forName() invocation. flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java <https://reviews.apache.org/r/4115/#comment12597> ConfigurationException should extend FlumeException. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4115/#comment12602> Actually creates a fully populated configuration. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4115/#comment12607> Need a few test cases to make sure this is working as expected. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4115/#comment12603> Misleading error. When value is null, the error should not be agent name missing. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4115/#comment12621> This exception is currently raised only if the instantiation fails. Ideally though, you want the conf object to be created and invoke a .validate() method on it, and then be able to extract any errors for reporting upwards. Same comment applies to validateSources etc. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java <https://reviews.apache.org/r/4115/#comment12612> Please use generics to ensure type compliance instead of casts. flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationError.java <https://reviews.apache.org/r/4115/#comment12604> This should have provision for value that is in question (since some errors are value specific), and also the raw property that triggered this. Otherwise the error could be ambiguous flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java <https://reviews.apache.org/r/4115/#comment12605> These enums should have a descriptive message that tell the user what this problem is. Something like: PROPERTY_NAME_NULL("A null or empty value was specified for the given property") etc. flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java <https://reviews.apache.org/r/4115/#comment12617> I don't think this is enum should have sink group and sink processor information. They belong to separate category. flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java <https://reviews.apache.org/r/4115/#comment12616> Please define an interface that this enum implements to provide the class name instead of overloading the toString() method. Same for other enums too. flume-ng-core/pom.xml <https://reviews.apache.org/r/4115/#comment12599> Indentation. - Arvind On 2012-03-09 08:47:00, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4115/ > ----------------------------------------------------------- > > (Updated 2012-03-09 08:47:00) > > > Review request for Flume. > > > Summary > ------- > > Currently the configuration of each component sits deep inside the component > themselves. There is no way to verify if a configuration is valid before run > time. In most systems like Flume, it is likely that these confs will be > deployed from one single host, on to the machines where flume agents are > running. Only when each agent starts, invalid confs are identified because > the Agents would terminate by throwing exceptions. This is a first attempt to > make a component-aware configuration system which is independent of the > Flume, and does not require the Flume jar to be installed. Each component > needs a configuration manager, which configures the components. > > I have not completed the validation part yet, but have completed the > configuration managers for all the components that come in the flume jar. I > will add support for the other components, such as HDFS sink, JDBC channel > etc soon. I will also add the validation support to the classes, but would > like a review of the model being proposed here. > > Note that I do know of a couple of bugs in the existing diff(like that if the > name of a config key starts with an Upper case letter, an exception is > thrown) - I will fix and upload the review here soon. > > > This addresses bug FLUME-992. > https://issues.apache.org/jira/browse/FLUME-992 > > > Diffs > ----- > > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/NetcatSourceConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/ExecSourceConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/AvroSourceConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorType.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkType.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/RollingFileSinkConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.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-configuration/src/main/java/org/apache/flume/conf/channel/ChannelConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelSelectorConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelSelectorType.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/MemoryChannelConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/MultiplexingChannelSelectorConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/PseudoTxnMemoryChannelConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/AvroSinkConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/FailoverSinkProcessorConfiguration.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/Context.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/ComponentConfigurationFactory.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java > PRE-CREATION > > flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java > 68d95fb > flume-ng-configuration/pom.xml PRE-CREATION > > flume-ng-channels/flume-jdbc-channel/src/test/java/org/apache/flume/channel/jdbc/TestJdbcChannelProvider.java > 7710d46 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java > a279453 > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannel.java > bca0c50 > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannelProvider.java > e445d61 > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannelProviderFactory.java > 6fbd6ef > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java > 307ae89 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceType.java > PRE-CREATION > flume-ng-configuration/src/test/resources/flume-ng-error-test PRE-CREATION > flume-ng-configuration/src/test/resources/flume-ng-test.conf PRE-CREATION > flume-ng-core/pom.xml d753fa1 > flume-ng-core/src/main/java/org/apache/flume/ChannelSelector.java fba2dcb > flume-ng-core/src/main/java/org/apache/flume/Context.java a6341a5 > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad > flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b > flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java > 352bf08 > > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java > 800f471 > > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorType.java > 511fc65 > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelType.java > d8419e8 > > flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java > 963a6a3 > flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > bfa1fde > > flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java > 83928b7 > > flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java > 489d3e5 > > flume-ng-core/src/main/java/org/apache/flume/channel/ReplicatingChannelSelector.java > 8f22746 > flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839 > > 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/main/java/org/apache/flume/sink/AvroSink.java 7386d06 > flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkFactory.java > b89dfa0 > flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkProcessor.java > 257bab3 > > flume-ng-core/src/main/java/org/apache/flume/sink/FailoverSinkProcessor.java > 9f5b856 > flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java > 7f1d3b3 > flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java > 10f9f4e > flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09 > flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java a3f6640 > > flume-ng-core/src/main/java/org/apache/flume/source/DefaultSourceFactory.java > a610e6f > flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java b6b1181 > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java > 94245ac > flume-ng-core/src/main/java/org/apache/flume/source/SourceType.java 9082470 > flume-ng-core/src/test/java/org/apache/flume/TestContext.java 51c350f > > flume-ng-core/src/test/java/org/apache/flume/channel/AbstractBasicChannelSemanticsTest.java > 6e71e46 > flume-ng-core/src/test/java/org/apache/flume/channel/MockChannel.java > 24b01e2 > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java > e070864 > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java > 8dad0b2 > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java > bc81f26 > flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 467785f > > flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java > 195c121 > 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-core/src/test/java/org/apache/flume/source/TestAvroSource.java > c5c3f2f > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java > 6035270 > > flume-ng-core/src/test/java/org/apache/flume/source/TestPollableSourceRunner.java > c27f82c > > flume-ng-core/src/test/java/org/apache/flume/source/TestSequenceGeneratorSource.java > 579b257 > > flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java > 65d9142 > > flume-ng-legacy-sources/flume-avro-source/src/test/java/org/apache/flume/source/avroLegacy/TestLegacyAvroSource.java > 6e3eb53 > > flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java > fbf7362 > > flume-ng-legacy-sources/flume-thrift-source/src/test/java/org/apache/flume/source/thriftLegacy/TestThriftLegacySource.java > ddd9478 > > 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 > 97f72e1 > > flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java > 521b586 > flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java > fb2a960 > flume-ng-node/src/test/resources/flume-conf.properties 848caca > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 524b69c > > flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java > 7d8ee8a > > flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java > 8e77218 > pom.xml d785762 > > Diff: https://reviews.apache.org/r/4115/diff > > > Testing > ------- > > All existing unit tests for the components whose configuration has now been > moved to these stubs pass. > > > Thanks, > > Hari > >
