----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3516/#review4829 -----------------------------------------------------------
I'm not seeing any unit tests attached to this? Personally I still feel that handling all checked exceptions other than InterruptedException should be the channels responsibility, and it should be their responsibility to decide whether to propagate them as ChannelExceptions or do something else. Regarding the counting, I personally found it to make semantics awkward and unclear(what do we do if we get two begins and one commits then the other rollbacks!?) so it's loss isn't a big deal to me. In fact I might even go so far as saying that I think a transaction should only be openable once, as they are now entirely threadlocal. These are just my personal opinions though and I think the patch should be good to go, and perhaps it can be further polished in a different issue. - Juhani On 2012-02-06 06:37:40, Peter Newcomb wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3516/ > ----------------------------------------------------------- > > (Updated 2012-02-06 06:37:40) > > > Review request for Flume. > > > Summary > ------- > > Implementation of FLUME-935 as new classes BasicChannelSemantics, > BasicTransactionSemantics, and ChannelUtils. It might be better to fold > BasicChannelSemantics into AbstractChannel and rename > BasicTransactionSemantics to AbstractTransaction, but doing that would > require refactoring of existing classes that extend AbstractChannel. > > > This addresses bug FLUME-935. > https://issues.apache.org/jira/browse/FLUME-935 > > > Diffs > ----- > > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicChannelSemantics.java > PRE-CREATION > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSemantics.java > PRE-CREATION > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3516/diff > > > Testing > ------- > > I am using these in production code, and they have survived significant > integration testing there, including failure modes. Note also that these > classes are largely error handling and precondition testing code designed to > test the correctness of the code around them. > > A fairly comprehensive set of unit tests around BasicChannelSemantics and > BasicTransactionSemantics is included. > > > Thanks, > > Peter > >
