I would suggest that when you manually reply to a review request like this that you change the subject as it still blends in with all the noise from the other review requests. Personally, I am beginning to dislike the review system as I can't really follow any conversations that take place there as the noise ratio of "Ship it" to other stuff is awful. I greatly appreciate discussions happening on the list directly.
Ralph On Apr 18, 2012, at 12:36 AM, Juhani Connolly wrote: > As suggested mailing straight to the dev list to avoid the review board noise. > > On 04/18/2012 12:59 PM, Hari Shreedharan wrote: >> >>> 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). > I'm aware a lot of it was old stuff moved around... I just reread the lot of > it because it was more effort to figure out what had changed than to get > everything. Certainly some of the issues may well have been there before and > a lot of changes for the better were made. > One of the main non dry parts I noticed was the getConfiguration() in the > stubs(SinkProcessorConfiguration/SinkConfiguration/SourceConfiguration/etc. > This could have the classname as a parameter(and the stubs have a short > function that just calls with the relevant classname) to avoid repeating the > code. I think it may be possible to do something with getKnownXYZ which > could also probably be parameterized to cut down on code. > >> >> >>> 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? > Not really :) >> >> - Hari >> >> > I locally applied the patch to trunk and all the tests passed. I think once > the new channels are added to the Type class along with the other little > fixes this should be good to go, though since it is pretty big, it would be > nice to get someone elses input too. > > Thanks, > Juhani
