[
https://issues.apache.org/jira/browse/FLUME-1052?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239678#comment-13239678
]
[email protected] commented on FLUME-1052:
------------------------------------------------------
-----------------------------------------------------------
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:
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-03-27 06:17:30)
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/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/Context.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-core/pom.xml 37fb112
bq. flume-ng-core/src/main/java/org/apache/flume/Context.java 5294e31
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/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java
3358cf4
bq. flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java
92ff6fe
bq.
flume-ng-core/src/test/java/org/apache/flume/sink/TestRollingFileSink.java
7e26e2a
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
50b9f0c
bq. pom.xml c91222f
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
