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

Details

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.

Activity

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 06:23
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 10:51
-----------------------------------------------------------
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

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 15:15
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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 15:15
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 18:57
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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 18:59
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

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 19:01
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

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 19:07
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

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 19:51
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

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.

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

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.

jiraposter@reviews.apache.org
added a comment - 05/May/12 20:19
-----------------------------------------------------------
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