[
https://issues.apache.org/jira/browse/FLUME-968?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13216305#comment-13216305
]
[email protected] commented on FLUME-968:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4010/#review5346
-----------------------------------------------------------
Hari,
I looked through the code, and ran it using a command line driver and a shell
script I hacked together.
First off this seems quite comprehensive in the errors it catches, which is
great. Nice work. Also, well done putting in all the javadoc comments. Flume
really needs more of those. The comments you wrote are very helpful to
understand what's going on in the code.
There are some additional methods that I think would be very helpful to add to
the API:
- A method to get a list of all the agents defined by the config in
FlumeConfiguration, i.e. List<String> getAgents()
- A method that returns the line in the file of the error found in
FlumeConfigurationError, i.e. int getErrorLine()
- A boolean method in FlumeConfiguration that simply tells us if it's a good
file, i.e. isValid() or conversely hasErrors()
General stylistic suggestions -- these are somewhat subjective, so just things
to consider:
- Consider renaming FlumeConfiguration to Configuration (we know it's for
Flume because of the package name)
- Consider FlumeConfigurationError -> ConfigurationError
- Consider making ConfigurationErrorType a public inner class (enum) of
ConfigurationError simply called Type
- Consider renaming ComponentNameAndConfigKey to ComponentInfo or something a
little more generic
I also left some comments inline in the review tool on the patch.
One last suggestion: In my view, a library should not have logging code in it.
The warnings and associated messages should be fully exposed via the returned
error objects from the validation APIs. Either that, or via Exceptions but I
think that the lists of error objects you have included in this design are the
right approach. But to have the full information of the warning messages they
would need a couple more fields, i.e. human-readable error message and line
number.
By the way, I can provide my simple driver code if you want, but we should have
something checked in that exercises this library code. The first client of this
could be a simple standalone validation tool.
Again, nice job with all of the validation. I think this contribution will be
very useful, not only due to the need to validate the configs but also because
other build tools & IDEs could add Flume support on top of it, which would be
great.
Regards,
Mike
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4010/#comment11652>
Can you make enums out of all of these private static final Strings? You
could do something like:
private enum Entity {
SOURCES("sources"),
SINKS("sinks"),
SINKGROUPS("sinkgroups"),
CHANNELS("channels");
private final String value;
Entity(String value) {
this.value = value;
}
@Override
public String toString() {
return value;
}
}
Then just add API support for the postfixed "." in parseConfigKey() or
something like that.
Same thing for the Attr and Class constants... good candidates for Enums.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4010/#comment11650>
Should be declared as the interface instead of the implementation. Could
also declare as final since you're instantiating it in your constructor.
private final List<FlumeConfigurationError> errors;
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4010/#comment11649>
You can use List<String> propertyNames = properties.stringPropertyNames()
in Java 1.6 and avoid the type cast here.
flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
<https://reviews.apache.org/r/4010/#comment11651>
Consider using the Java 1.6 for-each idiom, it tends to be more terse &
readable:
for (String agentName : agentConfigMap.keySet()) {
...
}
Usually this is preferred unless you need to use the iterator to modify the
collection, delete elements, etc.
- Mike
On 2012-02-24 00:29:31, Hari Shreedharan wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4010/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-24 00:29:31)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This is a first cut for the library that does the syntactical validation
of config files. I would like feedback on naming conventions for the errors and
better ways to pass data around. Basically this patch is meant to demonstrate
the fundamental idea.
bq.
bq.
bq. This addresses bug FLUME-968.
bq. https://issues.apache.org/jira/browse/FLUME-968
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/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/test/resources/flume-ng-error-test
PRE-CREATION
bq. flume-ng-configuration/src/test/resources/flume-ng-test.conf
PRE-CREATION
bq. flume-ng-node/pom.xml b9b062e
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. pom.xml d785762
bq.
bq. Diff: https://reviews.apache.org/r/4010/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. I will add tests once I get initial feedback.
bq.
bq.
bq. Thanks,
bq.
bq. Hari
bq.
bq.
> Need a library that would allow external tools to parse and validate flume
> properties file configuration
> --------------------------------------------------------------------------------------------------------
>
> Key: FLUME-968
> URL: https://issues.apache.org/jira/browse/FLUME-968
> Project: Flume
> Issue Type: Sub-task
> Affects Versions: v1.0.0
> Reporter: Arvind Prabhakar
> Fix For: v1.1.0
>
> Attachments: FLUME-969-2.patch
>
>
> 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.
--
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
