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