I did add a way to return the errors from FlumeConfiguration, which were just being logged, there was no change in functionality.
On Fri, Mar 2, 2012 at 11:55 PM, Hari Shreedharan <[email protected] > wrote: > > > 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 >> > >
