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
