----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4837/#review7474 -----------------------------------------------------------
Thanks for the patch Brock. Changes look good. I do have my tx-nit request below for you to consider. Also noted some other feedback. flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java <https://reviews.apache.org/r/4837/#comment16519> Perhaps this is equivalent to: ... } catch (Throwable th) { if (tx != null) { try { tx.rollback(); } catch (Exception ex) { LOG.warn("exception...", ex); } } Throwables.propagate(th); } finally { if (tx != null) { tx.close(); } } If you agree, I would request that we follow this logic instead in other places as well below. flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java <https://reviews.apache.org/r/4837/#comment16521> Missing the license header. flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java <https://reviews.apache.org/r/4837/#comment16520> Thanks for adding these tests. - Arvind On 2012-04-22 20:04:13, Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4837/ > ----------------------------------------------------------- > > (Updated 2012-04-22 20:04:13) > > > Review request for Flume. > > > Summary > ------- > > ChannelProcessor did not handle transactions well, in some cases if anything > but ChannelException was thrown, the close in the finally would throw an > exception as rollback was not called. In other cases if a subclass of error > was thrown the same would occurred. Additionally, if getTransaction threw an > exception the null transaction value was not handled and an NPE would be > thrown. > > > This addresses bug FLUME-1131. > https://issues.apache.org/jira/browse/FLUME-1131 > > > Diffs > ----- > > > flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java > PRE-CREATION > flume-ng-core/pom.xml b9f1e12 > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java > eb6460e > > Diff: https://reviews.apache.org/r/4837/diff > > > Testing > ------- > > Tests added and tests pass. > > > Thanks, > > Brock > >
