Review Request Noise

[Ralph Goers <ra...@dslextreme.com>]

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