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