You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Hari Shreedharan <hs...@cloudera.com> on 2012/03/27 08:17:30 UTC

Review Request: Main config component

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

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/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/Context.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-core/pom.xml 37fb112 
  flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 
  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/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 3358cf4 
  flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe 
  flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java 7e26e2a 
  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 50b9f0c 
  pom.xml c91222f 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Review Request Noise

Posted by 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


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
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
> 
> 



Re: Review Request: Main config component

Posted by Juhani Connolly <ju...@cyberagent.co.jp>.
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

Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.

> 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
> 
>


Re: Review Request: Main config component

Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review6970
-----------------------------------------------------------


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.


flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15524>

    s/PropertiesFileConfgurationProvider/PropertiesFileConfigurationProvider/



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15523>

    s/configuation/configuration/



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15526>

    do you think you could rename this to channelName for code clarity? Same thing for the validateSource/Sinks



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15527>

    ChannelType->SourceType



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15528>

    ChannelType-> SinkType



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment15529>

    I don't think this doc applies for the validateGroups as there is only one type of group configuration and it is handled here



flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java
<https://reviews.apache.org/r/4502/#comment15547>

    I believe you'll need to add in the new channels here


- Juhani


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
> 
>


Re: Review Request: Main config component

Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review7121
-----------------------------------------------------------

Ship it!


+1

Looks good to me too. Please attach the patch to the Jira.

- Arvind


On 2012-04-19 17:20:28, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-04-19 17:20:28)
> 
> 
> 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 b9f1e12 
>   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 917ac02 
>   flume-ng-dist/src/main/assembly/src.xml 3b24b39 
>   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 ed8092d 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-04-19 17:20:28.083324)


Review request for Flume.


Changes
-------

Adding recoverable memory channel to ChannelConfiguration too.


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  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 b9f1e12 
  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 917ac02 
  flume-ng-dist/src/main/assembly/src.xml 3b24b39 
  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 ed8092d 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Re: Review Request: Main config component

Posted by Juhani Connolly <ju...@gmail.com>.

> On 2012-04-19 07:02:57, Juhani Connolly wrote:
> > pom.xml, line 41
> > <https://reviews.apache.org/r/4502/diff/4-6/?file=101779#file101779line41>
> >
> >     These changes don't look like they belong here... Maybe your patch was based against the wrong version?
> 
> Hari Shreedharan wrote:
>     Hi Juhani,
>     
>     Thanks for the feedback. This appears in the diff because I rebased the patch on trunk (sorry, I may have forgotten to mention this). Please see : https://github.com/apache/flume/blob/trunk/pom.xml(lines 41 to 46). Thats the reason you see it as added when you diff between patch versions 4 and 6. This patch should be applied to trunk.

right, my bad!

Everything looks good to me with the addition of the other memory channel. I'll check over individual components as soon as this is committed


- Juhani


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review7018
-----------------------------------------------------------


On 2012-04-19 17:20:28, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-04-19 17:20:28)
> 
> 
> 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 b9f1e12 
>   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 917ac02 
>   flume-ng-dist/src/main/assembly/src.xml 3b24b39 
>   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 ed8092d 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.

> On 2012-04-19 07:02:57, Juhani Connolly wrote:
> > pom.xml, line 41
> > <https://reviews.apache.org/r/4502/diff/4-6/?file=101779#file101779line41>
> >
> >     These changes don't look like they belong here... Maybe your patch was based against the wrong version?

Hi Juhani,

Thanks for the feedback. This appears in the diff because I rebased the patch on trunk (sorry, I may have forgotten to mention this). Please see : https://github.com/apache/flume/blob/trunk/pom.xml(lines 41 to 46). Thats the reason you see it as added when you diff between patch versions 4 and 6. This patch should be applied to trunk.


- Hari


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review7018
-----------------------------------------------------------


On 2012-04-19 01:33:31, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-04-19 01:33:31)
> 
> 
> 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 b9f1e12 
>   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 917ac02 
>   flume-ng-dist/src/main/assembly/src.xml 3b24b39 
>   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 ed8092d 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review7018
-----------------------------------------------------------


Looks good to me except for some changes from another patch look like they've slipped in
Once that's fixed if you can attach the patch to the jira it should be good to go


pom.xml
<https://reviews.apache.org/r/4502/#comment15590>

    These changes don't look like they belong here... Maybe your patch was based against the wrong version?


- Juhani


On 2012-04-19 01:33:31, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-04-19 01:33:31)
> 
> 
> 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 b9f1e12 
>   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 917ac02 
>   flume-ng-dist/src/main/assembly/src.xml 3b24b39 
>   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 ed8092d 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-04-19 01:33:31.872414)


Review request for Flume.


Changes
-------

Incorporated Juhani's feedback. Last diff missed the new files.


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  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 b9f1e12 
  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 917ac02 
  flume-ng-dist/src/main/assembly/src.xml 3b24b39 
  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 ed8092d 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-04-19 01:23:41.296962)


Review request for Flume.


Changes
-------

Incorporated Juhani's feedback. For some reason, git seems to have reordered the files in the diff, sorry about that.


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  flume-ng-core/pom.xml b9f1e12 
  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/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 917ac02 
  flume-ng-dist/src/main/assembly/src.xml 3b24b39 
  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 ed8092d 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-04-13 21:52:25.670555)


Review request for Flume.


Changes
-------

Rebased on trunk. 


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  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


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-03-30 07:32:58.686432)


Review request for Flume.


Changes
-------

Adding many default features to the core conf patch. Flume node now works with even the built in components, even without config stubs.


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  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 9082470 
  flume-ng-dist/pom.xml 4c49452 
  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 50b9f0c 
  pom.xml c91222f 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/
-----------------------------------------------------------

(Updated 2012-03-29 21:58:26.193754)


Review request for Flume.


Changes
-------

Added the dependencies into this patch. Made sure it builds.


Summary
-------

Main config component.


This addresses bug FLUME-1052.
    https://issues.apache.org/jira/browse/FLUME-1052


Diffs (updated)
-----

  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/AbstractChannel.java 352bf08 
  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/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 9082470 
  flume-ng-core/src/test/java/org/apache/flume/channel/AbstractBasicChannelSemanticsTest.java 6e71e46 
  flume-ng-core/src/test/java/org/apache/flume/channel/MockChannel.java 24b01e2 
  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 50b9f0c 
  pom.xml c91222f 

Diff: https://reviews.apache.org/r/4502/diff


Testing
-------

Functional testing done.


Thanks,

Hari


Re: Review Request: Main config component

Posted by Hari Shreedharan <hs...@cloudera.com>.

> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, line 399
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line399>
> >
> >     Need to handle the case where the config is not specified, but the component is a standard component. In such cases, the component type enum should be used to determine the config holder class.

If it is a known component, the getKnownChannel function returns the ChannelType enum instance which points to that channel, and that is set to the config object. This config object is what is used to create the configuration object. If we have memory in the conf, chType will be ChannelType.MEMORY and toString() will return MEMORY. This is then used to create the MemoryChannelConfiguration object. 

Therefore, standard components are handled, so the case mentioned above works ok.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, line 312
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line312>
> >
> >     Looks like this check needs to happen before the if(channelSet.size() == 0) check.

Yes, will move it up.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, lines 300-301
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line300>
> >
> >     I see that this check has been disabled for various components. Without this, the active component check, required attribute checks will not happen. How is the code handling those right now?

Each of the basic configuration types - SourceConfiguration, SinkConfiguration, ChannelConfiguration etc. do the basic checks required for individual components.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, lines 88-90
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line88>
> >
> >     This seems redundant as the addRawProperty() method will perform these checks anyway.

I believe this is done in FlumeConfiguration even now, I will remove it anyway.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, lines 36-42
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line36>
> >
> >     These dependencies are missing. You need to move them into this module.

These are in a different patch, I will add them here.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/Context.java, line 20
> > <https://reviews.apache.org/r/4502/diff/1/?file=96765#file96765line20>
> >
> >     Please do not change the package as that will break backward compatibility.

Yes.will do.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java, line 21
> > <https://reviews.apache.org/r/4502/diff/1/?file=96764#file96764line21>
> >
> >     This should extend FlumeException to be consistent.

ok


- Hari


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review6429
-----------------------------------------------------------


On 2012-03-27 06:17:30, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-03-27 06:17:30)
> 
> 
> 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/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/Context.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-core/pom.xml 37fb112 
>   flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 
>   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/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 3358cf4 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java 7e26e2a 
>   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 50b9f0c 
>   pom.xml c91222f 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Arvind Prabhakar <ar...@apache.org>.

> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, lines 36-42
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line36>
> >
> >     These dependencies are missing. You need to move them into this module.
> 
> Hari Shreedharan wrote:
>     These are in a different patch, I will add them here.

Thanks. Otherwise I cannot compile your patch.


> On 2012-03-27 17:31:44, Arvind Prabhakar wrote:
> > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java, line 399
> > <https://reviews.apache.org/r/4502/diff/1/?file=96766#file96766line399>
> >
> >     Need to handle the case where the config is not specified, but the component is a standard component. In such cases, the component type enum should be used to determine the config holder class.
> 
> Hari Shreedharan wrote:
>     If it is a known component, the getKnownChannel function returns the ChannelType enum instance which points to that channel, and that is set to the config object. This config object is what is used to create the configuration object. If we have memory in the conf, chType will be ChannelType.MEMORY and toString() will return MEMORY. This is then used to create the MemoryChannelConfiguration object. 
>     
>     Therefore, standard components are handled, so the case mentioned above works ok.

Thanks for the explanation Hari.


- Arvind


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review6429
-----------------------------------------------------------


On 2012-03-27 06:17:30, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-03-27 06:17:30)
> 
> 
> 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/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/Context.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-core/pom.xml 37fb112 
>   flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 
>   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/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 3358cf4 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java 7e26e2a 
>   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 50b9f0c 
>   pom.xml c91222f 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>


Re: Review Request: Main config component

Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4502/#review6429
-----------------------------------------------------------


Thanks for the patch Hari. The changes look good. Some minor feedback follows:

* We follow the a variant of standard Java coding conventions. Please make sure your code is formatted accordingly. More details on this can be found at https://cwiki.apache.org/confluence/display/FLUME/How+to+Contribute#HowtoContribute-CodeQuality.


flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4502/#comment14024>

    This constant is defined twice - ComponentConfiguration and FlumeConfiguration. It will be better to externalize it in one constants class and reference it from there. Same for other constants too.



flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4502/#comment14023>

    please use different variable name as it is error prone to use names that shadow member variables.



flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4502/#comment14025>

    nit: suggest renaming this to failIfConfigured



flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java
<https://reviews.apache.org/r/4502/#comment14026>

    nit: suggest using a switch/case



flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java
<https://reviews.apache.org/r/4502/#comment14027>

    ("Cannot create configuration! Unknown Type specified: " + component)



flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java
<https://reviews.apache.org/r/4502/#comment14028>

    This should extend FlumeException to be consistent.



flume-ng-configuration/src/main/java/org/apache/flume/conf/Context.java
<https://reviews.apache.org/r/4502/#comment14022>

    Please do not change the package as that will break backward compatibility.



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment14021>

    These dependencies are missing. You need to move them into this module.



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment14029>

    This seems redundant as the addRawProperty() method will perform these checks anyway.



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment14032>

    I see that this check has been disabled for various components. Without this, the active component check, required attribute checks will not happen. How is the code handling those right now?



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment14030>

    Looks like this check needs to happen before the if(channelSet.size() == 0) check.



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4502/#comment14034>

    Need to handle the case where the config is not specified, but the component is a standard component. In such cases, the component type enum should be used to determine the config holder class.



flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java
<https://reviews.apache.org/r/4502/#comment14035>

    Please do not use exclamation marks in error messages.



flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java
<https://reviews.apache.org/r/4502/#comment14036>

    s/Configuring channels


- Arvind


On 2012-03-27 06:17:30, Hari Shreedharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated 2012-03-27 06:17:30)
> 
> 
> 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/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/Context.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-core/pom.xml 37fb112 
>   flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 
>   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/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 3358cf4 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe 
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java 7e26e2a 
>   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 50b9f0c 
>   pom.xml c91222f 
> 
> Diff: https://reviews.apache.org/r/4502/diff
> 
> 
> Testing
> -------
> 
> Functional testing done.
> 
> 
> Thanks,
> 
> Hari
> 
>