Hi Juhani, I think this can be updated/fixed once the main patch goes in. I have submitted a new patch with your feedback incorporated. Also, some of the diffs between the last 2 diffs in the pom file you see is due to the rebase of the file I did, these updates are already in trunk. Please take a look, when you get the time.
-- Hari Shreedharan On Wednesday, April 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 > >
