[ 
https://issues.apache.org/jira/browse/FLUME-1052?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13256183#comment-13256183
 ] 

[email protected] commented on FLUME-1052:
------------------------------------------------------



bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > I finally had time to go through this huge patch. Great work!
bq.  > 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.
bq.  > 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). 


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 line 50
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line50>
bq.  >
bq.  >     
s/PropertiesFileConfgurationProvider/PropertiesFileConfigurationProvider/

will fix it. 


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 line 133
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line133>
bq.  >
bq.  >     s/configuation/configuration/

will fix.


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 line 408
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line408>
bq.  >
bq.  >     do you think you could rename this to channelName for code clarity? 
Same thing for the validateSource/Sinks

ok. will do that.


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 line 480
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line480>
bq.  >
bq.  >     ChannelType->SourceType

ah copy-paste issue.


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 line 574
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line574>
bq.  >
bq.  >     ChannelType-> SinkType

same as above. will fix both


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java,
 lines 665-683
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101742#file101742line665>
bq.  >
bq.  >     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.


bq.  On 2012-04-18 03:11:55, Juhani Connolly wrote:
bq.  > 
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java,
 line 41
bq.  > <https://reviews.apache.org/r/4502/diff/4/?file=101748#file101748line41>
bq.  >
bq.  >     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:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4502/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-04-13 21:52:25)
bq.  
bq.  
bq.  Review request for Flume.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Main config component.
bq.  
bq.  
bq.  This addresses bug FLUME-1052.
bq.      https://issues.apache.org/jira/browse/FLUME-1052
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    flume-ng-configuration/pom.xml PRE-CREATION 
bq.    flume-ng-configuration/src/main/java/org/apache/flume/Context.java 
PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationError.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelSelectorConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelSelectorType.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkProcessorType.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkType.java 
PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java
 PRE-CREATION 
bq.    
flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceType.java
 PRE-CREATION 
bq.    flume-ng-core/pom.xml 37fb112 
bq.    flume-ng-core/src/main/java/org/apache/flume/ChannelSelector.java 
fba2dcb 
bq.    flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31 
bq.    flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java d863ed0 
bq.    flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java 
be1891b 
bq.    
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java
 800f471 
bq.    
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorType.java 
511fc65 
bq.    flume-ng-core/src/main/java/org/apache/flume/channel/ChannelType.java 
d8419e8 
bq.    
flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java 
963a6a3 
bq.    
flume-ng-core/src/main/java/org/apache/flume/conf/ConfigurableComponent.java 
PRE-CREATION 
bq.    flume-ng-core/src/main/java/org/apache/flume/conf/Configurables.java 
84492e5 
bq.    
flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkFactory.java 
b89dfa0 
bq.    
flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkProcessor.java 
257bab3 
bq.    flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 0dffd69 
bq.    
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 
6160a17 
bq.    flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09 
bq.    
flume-ng-core/src/main/java/org/apache/flume/source/DefaultSourceFactory.java 
a610e6f 
bq.    flume-ng-core/src/main/java/org/apache/flume/source/SourceType.java 
cd8991e 
bq.    flume-ng-dist/pom.xml 642e681 
bq.    flume-ng-dist/src/main/assembly/dist.xml 7e401ae 
bq.    flume-ng-dist/src/main/assembly/src.xml bd4f090 
bq.    
flume-ng-node/src/main/java/org/apache/flume/conf/properties/FlumeConfiguration.java
 d66f6d1 
bq.    
flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java
 1f0e8c6 
bq.    pom.xml 3a9bc42 
bq.  
bq.  Diff: https://reviews.apache.org/r/4502/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Functional testing done.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Hari
bq.  
bq.


                
> Core configuration component
> ----------------------------
>
>                 Key: FLUME-1052
>                 URL: https://issues.apache.org/jira/browse/FLUME-1052
>             Project: Flume
>          Issue Type: Sub-task
>    Affects Versions: v1.0.0
>            Reporter: Hari Shreedharan
>            Assignee: Hari Shreedharan
>             Fix For: v1.2.0
>
>
> Currently the configuration provider implementation encompasses all the 
> syntactic and structural validation rules for loading the configuration. 
> Externalizing this functionality to a library will allow external tools to 
> easily operate on flume configuration files and be able to help parse and 
> validate these files.
> Currently the configuration of each component sits deep inside the component 
> themselves. There is no way to verify if a configuration is valid before run 
> time. In most systems like Flume, it is likely that these confs will be 
> deployed from one single host, on to the machines where flume agents are 
> running. Only when each agent starts, invalid confs are identified because 
> the Agents would terminate by throwing exceptions. This is a first attempt to 
> make a component-aware configuration system which is independent of the 
> Flume, and does not require the Flume jar to be installed. Each component 
> needs a configuration manager, which configures the components. 
> Provide abstract Configuration stubs for each component type, sources, 
> channels, sinks, selectors etc, which are in the new package, independent on 
> ng-core. Now for each of the channels extend the abstract class and check the 
> config properties for each of the required parameters for that component, for 
> example: MultiplexingChannelSelectorConfigurator would look for default 
> channel etc. If a particular component does not have a configuration class 
> then let the current mechanism continue. 
> This will require implementation for each component, but it should not be too 
> complex. One additional advantage we get from this is that we can separate 
> out the config validation from the components into these stubs, but we will 
> still need to read the values out of the Context as we do currently(else we 
> end up making the configuration dependent on flume-ng-core itself which we 
> wanted to avoid). 
> A problem with this is making a change to the configuration would require 
> changes in the configuration classes and in the components also(where the 
> configuration is read and the component is actually configured). I could not 
> figure out a way of avoiding this.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to