On Fri, Mar 2, 2012 at 5:54 PM, Eric Sammer <[email protected]> wrote:
> I just read the diff and I strongly object to it for the following reasons: > > * It's *terribly* overcomplicated / overengineered. You're very close to > having reimplemented Spring bean configuration (without strict DI). > - Yes, it is a bit complex, but if we need to make sure configuration is validated without having access to the components themselves, the properties of the components have to be separated out into another class. I am making changes to make it simpler, removing reflection from the validation logic which was used to remove the code repetition. > * There's significant logic in constructors (a code smell). > This was a first cut, I am moving the logic into to other methods. * Configuration is now highly coupled to the storage format (java > properties) > I am not sure how this is different from the current implementation. I did not add any of the Context methods or FlumeConfiguration classes (all I did in this patch was move it to the new package) - these are already based on Properties.. Please see: http://svn.apache.org/viewvc/incubator/flume/branches/flume-728/flume-ng-node/src/main/java/org/apache/flume/conf/properties/and http://svn.apache.org/viewvc/incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Context.java?revision=1294969&view=markup * FlumeConfiguration has become a God Object. > No changes were made to FlumeConfiguration at all. The only thing I did was to move it from the current package to the conf package. This is the exact same code as it is currently. It was already a god object. > * A bunch of hoops are jumped to remain type safe, and then the rest of > the code breaks all type safety by stringifying a ton of stuff. I'm not > specifically against dynamic techniques or reflection, but I'd like to see > it only when there's a true need and it adds real value. > Can you give me an example of the stringification that you have mentioned? I did not get this comment. If you are talking about reflection to convert the data into the correct type: Since the data would be read in through Context and it is type safe and since we use the same type it should not cause an issue. Yes, this can break if the classes are specified in correctly, but that is likely due to a bug. Either way, I have removed the reflection from the validation logic, as performance can be bad, but I don't specifically see a problem with it, as long as the code is correct. I am moving the properties for each component into the Conf classes(only the properties which are read from a configuration file or configured in some other way), from where the component reads this data using getters(To avoid 2 copies of the properties, they will not be duplicated in the components themselves). This will allow the validation to happen without having access to components. If you feel separating out the configured properties from the component is not a good idea, I am open to any suggestions on how to do the validation without actually being dependent on flume-core. Thanks Hari > On Thu, Mar 1, 2012 at 1:35 PM, Hari Shreedharan < > [email protected]> wrote: > >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/4115/ >> ----------------------------------------------------------- >> >> (Updated 2012-03-01 21:35:41.679397) >> >> >> Review request for Flume. >> >> >> Changes >> ------- >> >> 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. >> >> >> Summary (updated) >> ------- >> >> 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-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 >> 1cf1c0c >> >> flume-ng-channels/flume-jdbc-channel/src/test/java/org/apache/flume/channel/jdbc/TestJdbcChannelProvider.java >> 7710d46 >> >> 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-configuration/src/main/java/org/apache/flume/conf/AbstractComponentConfiguration.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-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/sink/RollingFileSinkConfiguration.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/source/AvroSourceConfiguration.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/NetcatSourceConfiguration.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/Context.java f1c8f85 >> 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 >> a7d5f94 >> flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java >> e79490e >> >> flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java >> c63d0a1 >> >> flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java >> 1df580e >> >> 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/Configurables.java >> 84492e5 >> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631 >> 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 >> 45c031d >> flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff >> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java >> 859f4fd >> flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java >> 7b079f9 >> flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java >> a96016c >> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java >> d205bbc >> flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8 >> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java >> 3392dff >> >> 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 >> 93ad3bf >> 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 >> b1e67f7 >> >> 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 >> 5fe270a >> >> flume-ng-legacy-sources/flume-thrift-source/src/test/java/org/apache/flume/source/thriftLegacy/TestThriftLegacySource.java >> ddd9478 >> flume-ng-node/pom.xml b9b062e >> >> 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-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java >> 3da90a5 >> >> 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 >> 0a5498f >> 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 >> >> > > > -- > Eric Sammer > twitter: esammer > data: www.cloudera.com >
