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/02/23 09:48:17 UTC

Review Request: Library to syntax verification of confs.

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

Review request for Flume.


Summary
-------

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.


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


Diffs
-----

  flume-ng-node/pom.xml b9b062e 
  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 97f72e1 
  pom.xml d785762 

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


Testing
-------

I will add tests once I get initial feedback.


Thanks,

Hari

Re: Review Request: Library to syntax verification of confs.

Posted by Mike Percy <mp...@gmail.com>.

-----------------------------------------------------------
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:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4010/
> -----------------------------------------------------------
> 
> (Updated 2012-02-24 00:29:31)
> 
> 
> Review request for Flume.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug FLUME-968.
>     https://issues.apache.org/jira/browse/FLUME-968
> 
> 
> Diffs
> -----
> 
>   flume-ng-configuration/pom.xml 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/test/resources/flume-ng-error-test PRE-CREATION 
>   flume-ng-configuration/src/test/resources/flume-ng-test.conf PRE-CREATION 
>   flume-ng-node/pom.xml b9b062e 
>   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 97f72e1 
>   pom.xml d785762 
> 
> Diff: https://reviews.apache.org/r/4010/diff
> 
> 
> Testing
> -------
> 
> I will add tests once I get initial feedback.
> 
> 
> Thanks,
> 
> Hari
> 
>

Re: Review Request: Library to syntax verification of confs.

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

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

(Updated 2012-02-24 00:29:31.312208)

Review request for Flume.

Changes
-------

Added a unit test to demonstrate basic idea. I welcome implementation improvements. The idea I am planning to implement is fundamentally to return the errors once the processing is done.

Summary
-------

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.

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

Diffs (updated)
-----

flume-ng-configuration/pom.xml 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/test/resources/flume-ng-error-test PRE-CREATION
flume-ng-configuration/src/test/resources/flume-ng-test.conf PRE-CREATION
flume-ng-node/pom.xml b9b062e
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 97f72e1
pom.xml d785762

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

Testing
-------

I will add tests once I get initial feedback.

Thanks,

Hari

Re: Review Request: Library to syntax verification of confs.

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

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

(Updated 2012-02-23 09:07:56.494266)


Review request for Flume.


Summary
-------

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.


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


Diffs
-----

  flume-ng-configuration/pom.xml 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-node/pom.xml b9b062e 
  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 97f72e1 
  pom.xml d785762 

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


Testing
-------

I will add tests once I get initial feedback.


Thanks,

Hari

Re: Review Request: Library to syntax verification of confs.

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

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

(Updated 2012-02-23 09:07:12.776308)


Review request for Flume.


Changes
-------

All changes


Summary
-------

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.


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


Diffs (updated)
-----

  flume-ng-configuration/pom.xml 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-node/pom.xml b9b062e 
  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 97f72e1 
  pom.xml d785762 

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


Testing
-------

I will add tests once I get initial feedback.


Thanks,

Hari

Re: Review Request: Library to syntax verification of confs.

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

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

(Updated 2012-02-23 08:53:47.389553)


Review request for Flume.


Changes
-------

Missed the new files which were added.


Summary
-------

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.


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


Diffs (updated)
-----

  flume-ng-configuration/pom.xml 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 

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


Testing
-------

I will add tests once I get initial feedback.


Thanks,

Hari