Flume
  1. Flume
  2. FLUME-1181

Context must enforce dot-separated prefix for sub-properties.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: v1.2.0
    • Component/s: Configuration
    • Labels:
      None

      Description

      Currently the method Context.getSubProperties(String prefix) assumes a well-formed prefix. This can lead to undetected issues such as what is being seen in the current state of channel selector configuration, where the keys within the channel selector are being computed as prefixed with a period.

      1. FLUME-1181-2.patch
        35 kB
        Arvind Prabhakar
      2. FLUME-1181-1.patch
        35 kB
        Arvind Prabhakar

        Activity

        Arvind Prabhakar created issue -
        Arvind Prabhakar made changes -
        Field Original Value New Value
        Attachment FLUME-1181-1.patch [ 12525722 ]
        Arvind Prabhakar made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Fix Version/s v1.2.0 [ 12320243 ]
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary
        -------

        This change:

        • introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,
        • fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken
        • refactors some of the source (not all) to externalize the configuration keys into separate constants class

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

        Diffs


        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310
        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310
        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310
        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310
        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310
        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing
        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Thanks for the patch, Arvind! I have some feedback below.

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java
        <https://reviews.apache.org/r/5017/#comment16809>

        This constant is defined twice. What is the difference between CONFIG_SOURCE_CHANNELSELECTOR_PREFIX and CONFIG_SELECTOR_PREFIX?

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java
        <https://reviews.apache.org/r/5017/#comment16810>

        This should be CONFIG_CHANNELS.

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
        <https://reviews.apache.org/r/5017/#comment16811>

        Not related to your change, but we might want to catch a NumberFormatException here. Same for even the eventSize getInteger call.

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
        <https://reviews.apache.org/r/5017/#comment16812>

        Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself.

        • Hari

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- Thanks for the patch, Arvind! I have some feedback below. /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java < https://reviews.apache.org/r/5017/#comment16809 > This constant is defined twice. What is the difference between CONFIG_SOURCE_CHANNELSELECTOR_PREFIX and CONFIG_SELECTOR_PREFIX? /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java < https://reviews.apache.org/r/5017/#comment16810 > This should be CONFIG_CHANNELS. /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java < https://reviews.apache.org/r/5017/#comment16811 > Not related to your change, but we might want to catch a NumberFormatException here. Same for even the eventSize getInteger call. /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java < https://reviews.apache.org/r/5017/#comment16812 > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself. Hari On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129

        > <https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129>

        >

        > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself.

        I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire.

        • Brock

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129 > < https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129 > > > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself. I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire. Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        Other than the items Hair mentioned, it looks good to me!

        • Brock

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7603 ----------------------------------------------------------- Ship it! Other than the items Hair mentioned, it looks good to me! Brock On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 15:14:29, Brock Noland wrote:

        > Other than the items Hair mentioned, it looks good to me!

        s/Hari/Hari

        • Hari

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 15:14:29, Brock Noland wrote: > Other than the items Hair mentioned, it looks good to me! s/Hari/Hari Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7603 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 15:14:29, Brock Noland wrote:

        > Other than the items Hair mentioned, it looks good to me!

        Hari Shreedharan wrote:

        s/Hari/Hari

        Sorry!! I will sheepishly hide behind the fact I had not yet drank my morning stimulus.

        • Brock

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 15:14:29, Brock Noland wrote: > Other than the items Hair mentioned, it looks good to me! Hari Shreedharan wrote: s/Hari/Hari Sorry!! I will sheepishly hide behind the fact I had not yet drank my morning stimulus. Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7603 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129

        > <https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129>

        >

        > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself.

        Brock Noland wrote:

        I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire.

        Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException.

        • Hari

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129 > < https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129 > > > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself. Brock Noland wrote: I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire. Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException. Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129

        > <https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129>

        >

        > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself.

        Brock Noland wrote:

        I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire.

        Hari Shreedharan wrote:

        Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException.

        We might be communicating the same thing in different words. I'd prefer that it's completely thrown from the configure method and the configure caller logs and disables the sink/source/channel. This way it won't work at all and someone will immediately know there is a problem as opposed to having to look at the logs to see if the configuration changes were applied.

        • Brock

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129 > < https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129 > > > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself. Brock Noland wrote: I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire. Hari Shreedharan wrote: Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException. We might be communicating the same thing in different words. I'd prefer that it's completely thrown from the configure method and the configure caller logs and disables the sink/source/channel. This way it won't work at all and someone will immediately know there is a problem as opposed to having to look at the logs to see if the configuration changes were applied. Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129

        > <https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129>

        >

        > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself.

        Brock Noland wrote:

        I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire.

        Hari Shreedharan wrote:

        Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException.

        Brock Noland wrote:

        We might be communicating the same thing in different words. I'd prefer that it's completely thrown from the configure method and the configure caller logs and disables the sink/source/channel. This way it won't work at all and someone will immediately know there is a problem as opposed to having to look at the logs to see if the configuration changes were applied.

        Yes. That is what I was saying as well, just thought better to wrap it up where it is initially thrown itself.

        • Hari

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java, line 129 > < https://reviews.apache.org/r/5017/diff/1/?file=106967#file106967line129 > > > Same as TCP source, we might want to handle NumberFormatExceptions for all getInteger calls. Maybe we should do that in Context itself. Brock Noland wrote: I prefer throwing any NFE from context as it can silently hide problems where you thought the configuration was set appropriately. I think it's better to fail to start to let the user know things are not correct as opposed to continuing with a configuration they did not desire. Hari Shreedharan wrote: Brock - I was not suggesting that we not throw from Context. We could wrap them up and throw, but on the other hand it is better that the calling code would know exactly why it failed without using getCause(). Either way is fine, I suggested this because of the fact that every getInteger call seems to need an NFE catch clause, but if it isnt this is will still need a catch with FlumException. Brock Noland wrote: We might be communicating the same thing in different words. I'd prefer that it's completely thrown from the configure method and the configure caller logs and disables the sink/source/channel. This way it won't work at all and someone will immediately know there is a problem as opposed to having to look at the logs to see if the configuration changes were applied. Yes. That is what I was saying as well, just thought better to wrap it up where it is initially thrown itself. Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java, line 36

        > <https://reviews.apache.org/r/5017/diff/1/?file=106958#file106958line36>

        >

        > This constant is defined twice. What is the difference between CONFIG_SOURCE_CHANNELSELECTOR_PREFIX and CONFIG_SELECTOR_PREFIX?

        removed this.

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java, line 57

        > <https://reviews.apache.org/r/5017/diff/1/?file=106962#file106962line57>

        >

        > This should be CONFIG_CHANNELS.

        good catch (phew)! fixed this.

        On 2012-05-05 10:51:23, Hari Shreedharan wrote:

        > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java, line 144

        > <https://reviews.apache.org/r/5017/diff/1/?file=106966#file106966line144>

        >

        > Not related to your change, but we might want to catch a NumberFormatException here. Same for even the eventSize getInteger call.

        Undersand your suggestion here and for the next comment Hari, but am inclined to defer this for later.

        • Arvind

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

        On 2012-05-05 06:22:46, Arvind Prabhakar wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5017/

        -----------------------------------------------------------

        (Updated 2012-05-05 06:22:46)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Summary

        -------

        This change:

        * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,

        * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken

        * refactors some of the source (not all) to externalize the configuration keys into separate constants class

        This addresses bug FLUME-1181.

        https://issues.apache.org/jira/browse/FLUME-1181

        Diffs

        -----

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310

        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310

        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310

        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310

        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310

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

        Testing

        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java, line 36 > < https://reviews.apache.org/r/5017/diff/1/?file=106958#file106958line36 > > > This constant is defined twice. What is the difference between CONFIG_SOURCE_CHANNELSELECTOR_PREFIX and CONFIG_SELECTOR_PREFIX? removed this. On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java, line 57 > < https://reviews.apache.org/r/5017/diff/1/?file=106962#file106962line57 > > > This should be CONFIG_CHANNELS. good catch (phew)! fixed this. On 2012-05-05 10:51:23, Hari Shreedharan wrote: > /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java, line 144 > < https://reviews.apache.org/r/5017/diff/1/?file=106966#file106966line144 > > > Not related to your change, but we might want to catch a NumberFormatException here. Same for even the eventSize getInteger call. Undersand your suggestion here and for the next comment Hari, but am inclined to defer this for later. Arvind ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/#review7595 ----------------------------------------------------------- On 2012-05-05 06:22:46, Arvind Prabhakar wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 06:22:46) Review request for Flume, Brock Noland and Hari Shreedharan. Summary ------- This change: * introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, * fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken * refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs ----- /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334310 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334310 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334310 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334310 /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334310 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-05-05 20:18:36.840078)

        Review request for Flume, Brock Noland and Hari Shreedharan.

        Changes
        -------

        Thanks Hari and Brock for the review and feedback. This diff incorporates the feedback and rebases the patch on the latest trunk.

        Summary
        -------

        This change:

        • introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period,
        • fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken
        • refactors some of the source (not all) to externalize the configuration keys into separate constants class

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

        Diffs (updated)


        /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334473
        /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334473
        /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334473
        /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334473
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334473
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334473
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334473
        /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334473

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

        Testing
        -------

        Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes.

        Also done some manual validation of the system with a few simple scenarios.

        Thanks,

        Arvind

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5017/ ----------------------------------------------------------- (Updated 2012-05-05 20:18:36.840078) Review request for Flume, Brock Noland and Hari Shreedharan. Changes ------- Thanks Hari and Brock for the review and feedback. This diff incorporates the feedback and rebases the patch on the latest trunk. Summary ------- This change: introduces a precondition check in Context.getSubProperties(String prefix) method enforce that the prefix ends with a period, fixes the TestPropertiesFileConfigurationProvider to load the property file which was previously not working, and fixes cases that are broken refactors some of the source (not all) to externalize the configuration keys into separate constants class This addresses bug FLUME-1181 . https://issues.apache.org/jira/browse/FLUME-1181 Diffs (updated) /trunk/flume-ng-node/src/test/resources/flume-conf.properties 1334473 /trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java 1334473 /trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java 1334473 /trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java 1334473 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java 1334473 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java 1334473 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java PRE-CREATION /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java 1334473 /trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java 1334473 Diff: https://reviews.apache.org/r/5017/diff Testing ------- Ran all tests. The updated TestPropertiesFileConfigurationProvider catches two issues now - the one with channel selector configuration not being set correctly, and the other a similar issue with syslog source format configuration. Both of these issues have been fixed with the changes. Also done some manual validation of the system with a few simple scenarios. Thanks, Arvind
        Arvind Prabhakar made changes -
        Attachment FLUME-1181-2.patch [ 12525749 ]
        Hide
        Brock Noland added a comment -

        Committed in 1334679. Thank you for your patch Arvind!

        Show
        Brock Noland added a comment - Committed in 1334679. Thank you for your patch Arvind!
        Brock Noland made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in flume-trunk #197 (See https://builds.apache.org/job/flume-trunk/197/)
        FLUME-1181: Context must enforce dot-separated prefix for sub-properties

        (Arvind Prabhakar via Brock Noland) (Revision 1334679)

        Result = SUCCESS
        brock : http://svn.apache.org/viewvc/?view=rev&rev=1334679
        Files :

        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java
        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java
        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java
        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java
        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java
        • /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
        • /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
        • /incubator/flume/trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java
        • /incubator/flume/trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java
        • /incubator/flume/trunk/flume-ng-node/src/test/resources/flume-conf.properties
        • /incubator/flume/trunk/flume-ng-sdk/src/test/resources
        • /incubator/flume/trunk/flume-ng-sdk/src/test/resources/log4j.properties
        Show
        Hudson added a comment - Integrated in flume-trunk #197 (See https://builds.apache.org/job/flume-trunk/197/ ) FLUME-1181 : Context must enforce dot-separated prefix for sub-properties (Arvind Prabhakar via Brock Noland) (Revision 1334679) Result = SUCCESS brock : http://svn.apache.org/viewvc/?view=rev&rev=1334679 Files : /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/Context.java /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkGroupConfiguration.java /incubator/flume/trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogSourceConfigurationConstants.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java /incubator/flume/trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java /incubator/flume/trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java /incubator/flume/trunk/flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java /incubator/flume/trunk/flume-ng-node/src/test/resources/flume-conf.properties /incubator/flume/trunk/flume-ng-sdk/src/test/resources /incubator/flume/trunk/flume-ng-sdk/src/test/resources/log4j.properties

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development