[
https://issues.apache.org/jira/browse/FLUME-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13226302#comment-13226302
]
[email protected] commented on FLUME-992:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4115/#review5783
-----------------------------------------------------------
Thanks for the interim patch Hari. I did a quick walk through of these changes
and have some feedback that follows. Some general issues:
* The component configuration stubs do not report any configuration errors at
this stage. While we cannot expect these stubs to do runtime validation, we
should at least check all the static rules that may be being violated. For
example a non-numeric port value is something that does not get caught/reported
by the current implementation.
* As implemented, the configuration validation mechanism only reports errors.
It should also report warnings where default values override the specified
values etc.
More specific feedback below:
flume-ng-configuration/pom.xml
<https://reviews.apache.org/r/4115/#comment12598>
Better to call it Flume NG Configuration only.
flume-ng-configuration/pom.xml
<https://reviews.apache.org/r/4115/#comment12600>
Duplicate
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4115/#comment12623>
should be private
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4115/#comment12624>
Does not look like its being used, in which case it should be removed
considering the risk it introduces.
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
<https://reviews.apache.org/r/4115/#comment12625>
This method is not returning any configuration values. Assuming it is work
in progress - you should have a way by which the specialized component classes
can use this without having to copy/rewrite this method.
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java
<https://reviews.apache.org/r/4115/#comment12614>
Please make a ComponentTypeEnum that has SOURCE, CHANNEL, SINKPROCESSOR,
CHANNELSELECTOR etc in it.
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java
<https://reviews.apache.org/r/4115/#comment12615>
Shouldn't this be Class<SinkConfiguration> ?
Ideally, you want to have an enum that can return the actual class instance
directly rather than doing a Class.forName() invocation.
flume-ng-configuration/src/main/java/org/apache/flume/conf/ConfigurationException.java
<https://reviews.apache.org/r/4115/#comment12597>
ConfigurationException should extend FlumeException.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4115/#comment12602>
Actually creates a fully populated configuration.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4115/#comment12607>
Need a few test cases to make sure this is working as expected.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4115/#comment12603>
Misleading error. When value is null, the error should not be agent name
missing.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4115/#comment12621>
This exception is currently raised only if the instantiation fails. Ideally
though, you want the conf object to be created and invoke a .validate() method
on it, and then be able to extract any errors for reporting upwards.
Same comment applies to validateSources etc.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4115/#comment12612>
Please use generics to ensure type compliance instead of casts.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationError.java
<https://reviews.apache.org/r/4115/#comment12604>
This should have provision for value that is in question (since some errors
are value specific), and also the raw property that triggered this. Otherwise
the error could be ambiguous
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfigurationErrorType.java
<https://reviews.apache.org/r/4115/#comment12605>
These enums should have a descriptive message that tell the user what this
problem is. Something like:
PROPERTY_NAME_NULL("A null or empty value was specified for the given
property")
etc.
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java
<https://reviews.apache.org/r/4115/#comment12617>
I don't think this is enum should have sink group and sink processor
information. They belong to separate category.
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java
<https://reviews.apache.org/r/4115/#comment12616>
Please define an interface that this enum implements to provide the class
name instead of overloading the toString() method. Same for other enums too.
flume-ng-core/pom.xml
<https://reviews.apache.org/r/4115/#comment12599>
Indentation.
- Arvind
On 2012-03-09 08:47:00, Hari Shreedharan wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4115/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-09 08:47:00)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. I have not completed the validation part yet, but have completed the
configuration managers for all the components that come in the flume jar. I
will add support for the other components, such as HDFS sink, JDBC channel etc
soon. I will also add the validation support to the classes, but would like a
review of the model being proposed here.
bq.
bq. Note that I do know of a couple of bugs in the existing diff(like that if
the name of a config key starts with an Upper case letter, an exception is
thrown) - I will fix and upload the review here soon.
bq.
bq.
bq. This addresses bug FLUME-992.
bq. https://issues.apache.org/jira/browse/FLUME-992
bq.
bq.
bq. Diffs
bq. -----
bq.
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/NetcatSourceConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/source/ExecSourceConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/source/AvroSourceConfiguration.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/sink/SinkProcessorConfiguration.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/RollingFileSinkConfiguration.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/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/channel/MemoryChannelConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/MultiplexingChannelSelectorConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/PseudoTxnMemoryChannelConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/AvroSinkConfiguration.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/FailoverSinkProcessorConfiguration.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/Context.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/ComponentConfigurationFactory.java
PRE-CREATION
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
PRE-CREATION
bq.
flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java
68d95fb
bq. flume-ng-configuration/pom.xml PRE-CREATION
bq.
flume-ng-channels/flume-jdbc-channel/src/test/java/org/apache/flume/channel/jdbc/TestJdbcChannelProvider.java
7710d46
bq.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
a279453
bq.
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannel.java
bca0c50
bq.
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannelProvider.java
e445d61
bq.
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannelProviderFactory.java
6fbd6ef
bq.
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java
307ae89
bq.
flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceType.java
PRE-CREATION
bq. flume-ng-configuration/src/test/resources/flume-ng-error-test
PRE-CREATION
bq. flume-ng-configuration/src/test/resources/flume-ng-test.conf
PRE-CREATION
bq. flume-ng-core/pom.xml d753fa1
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 a6341a5
bq. flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad
bq. flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java
be1891b
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java
352bf08
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/channel/MemoryChannel.java
bfa1fde
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java
83928b7
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java
489d3e5
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/ReplicatingChannelSelector.java
8f22746
bq. flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java
0fa4839
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/AvroSink.java 7386d06
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/FailoverSinkProcessor.java
9f5b856
bq. flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java
7f1d3b3
bq. flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56
bq.
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
10f9f4e
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/AvroSource.java
a3f6640
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/ExecSource.java
b6b1181
bq. flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java
94245ac
bq. flume-ng-core/src/main/java/org/apache/flume/source/SourceType.java
9082470
bq. flume-ng-core/src/test/java/org/apache/flume/TestContext.java 51c350f
bq.
flume-ng-core/src/test/java/org/apache/flume/channel/AbstractBasicChannelSemanticsTest.java
6e71e46
bq. flume-ng-core/src/test/java/org/apache/flume/channel/MockChannel.java
24b01e2
bq.
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java
e070864
bq.
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java
8dad0b2
bq.
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java
bc81f26
bq. flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java
467785f
bq.
flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java
195c121
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-core/src/test/java/org/apache/flume/source/TestAvroSource.java
c5c3f2f
bq. flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java
6035270
bq.
flume-ng-core/src/test/java/org/apache/flume/source/TestPollableSourceRunner.java
c27f82c
bq.
flume-ng-core/src/test/java/org/apache/flume/source/TestSequenceGeneratorSource.java
579b257
bq.
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java
65d9142
bq.
flume-ng-legacy-sources/flume-avro-source/src/test/java/org/apache/flume/source/avroLegacy/TestLegacyAvroSource.java
6e3eb53
bq.
flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java
fbf7362
bq.
flume-ng-legacy-sources/flume-thrift-source/src/test/java/org/apache/flume/source/thriftLegacy/TestThriftLegacySource.java
ddd9478
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
97f72e1
bq.
flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java
521b586
bq.
flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java
fb2a960
bq. flume-ng-node/src/test/resources/flume-conf.properties 848caca
bq.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java
524b69c
bq.
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSink.java
7d8ee8a
bq.
flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java
8e77218
bq. pom.xml d785762
bq.
bq. Diff: https://reviews.apache.org/r/4115/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing unit tests for the components whose configuration has now
been moved to these stubs pass.
bq.
bq.
bq. Thanks,
bq.
bq. Hari
bq.
bq.
> Create configuration stubs for sources, channels, sinks etc
> -----------------------------------------------------------
>
> Key: FLUME-992
> URL: https://issues.apache.org/jira/browse/FLUME-992
> Project: Flume
> Issue Type: Sub-task
> Affects Versions: v1.0.0
> Reporter: Hari Shreedharan
> Assignee: Hari Shreedharan
> Fix For: v1.1.0
>
>
> 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
