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