Flume
  1. Flume
  2. FLUME-991 Make flume configuration validation component specific at time rather than at runtime
  3. FLUME-968

Need a library that would allow external tools to parse and validate flume properties file configuration

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: v1.0.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      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.

      1. FLUME-969-2.patch
        68 kB
        Hari Shreedharan

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        Hari Shreedharan added a comment - - edited

        Please see FLUME-992 for a patch that covers this as well. I cannot remove the patch, but I don't think this needs to be committed - only FLUME-992 needs to be.

        Show
        Hari Shreedharan added a comment - - edited Please see FLUME-992 for a patch that covers this as well. I cannot remove the patch, but I don't think this needs to be committed - only FLUME-992 needs to be.
        Hide
        Hari Shreedharan added a comment -

        FLUME-992 addresses this issue.

        Show
        Hari Shreedharan added a comment - FLUME-992 addresses this issue.

          People

          • Assignee:
            Hari Shreedharan
            Reporter:
            Arvind Prabhakar
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development