> On 2012-04-18 03:11:55, Juhani Connolly wrote: > > I finally had time to go through this huge patch. Great work! > > In general it looks good, with a small number of minor niggles. Hopefully > > we can get this in before more major changes take place on the trunk. > > One other issue I have is that there is a lot of non-DRY code that looks > > like editted copy-paste. It may be a bit much to fix now, but hopefully in > > the future it can be refactored.
Thanks Juhani! Actually there are a bunch of files which were simply moved, not changed, but review board and git see them as new files added and old ones deleted. Also files like FlumeConfiguration, which was already huge was quite a lot refactored and some functions were rewritten. If you can give me some examples of the non-dry code you were talking about, I will take a look at it (over email is better - you can send it to the flume dev list, because reviewboard tends to create a lot of noise, here and on the dev list). > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > line 50 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line50> > > > > > > s/PropertiesFileConfgurationProvider/PropertiesFileConfigurationProvider/ will fix it. > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > line 133 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line133> > > > > s/configuation/configuration/ will fix. > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > line 408 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line408> > > > > do you think you could rename this to channelName for code clarity? > > Same thing for the validateSource/Sinks ok. will do that. > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > line 480 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line480> > > > > ChannelType->SourceType ah copy-paste issue. > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > line 574 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line574> > > > > ChannelType-> SinkType same as above. will fix both > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, > > lines 665-683 > > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line665> > > > > I don't think this doc applies for the validateGroups as there is only > > one type of group configuration and it is handled here yes. true. > On 2012-04-18 03:11:55, Juhani Connolly wrote: > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java, > > line 41 > > <https://reviews.apache.org/r/4502/diff/4/?file=101748#file101748line41> > > > > I believe you'll need to add in the new channels here Yes, I need to add recoverable memory channel. Do you have a preference for a nickname? - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4502/#review6970 ----------------------------------------------------------- On 2012-04-13 21:52:25, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4502/ > ----------------------------------------------------------- > > (Updated 2012-04-13 21:52:25) > > > 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/Context.java > 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/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/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/sink/SinkConfiguration.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/SinkProcessorConfiguration.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/source/SourceConfiguration.java > PRE-CREATION > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceType.java > PRE-CREATION > flume-ng-core/pom.xml 37fb112 > flume-ng-core/src/main/java/org/apache/flume/ChannelSelector.java fba2dcb > flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java d863ed0 > flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b > > 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/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/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/SinkGroup.java 0dffd69 > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java > 6160a17 > flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09 > > flume-ng-core/src/main/java/org/apache/flume/source/DefaultSourceFactory.java > a610e6f > flume-ng-core/src/main/java/org/apache/flume/source/SourceType.java cd8991e > flume-ng-dist/pom.xml 642e681 > flume-ng-dist/src/main/assembly/dist.xml 7e401ae > flume-ng-dist/src/main/assembly/src.xml bd4f090 > > 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 > 1f0e8c6 > pom.xml 3a9bc42 > > Diff: https://reviews.apache.org/r/4502/diff > > > Testing > ------- > > Functional testing done. > > > Thanks, > > Hari > >
