----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3516/#review4697 -----------------------------------------------------------
Thanks for the patch Peter. The framework looks good to me. A couple of questions/comments: 1. For reference counting the transaction, it is probably better to do decrement the nesting count within close() instead of commit/rollback. This is because the idiom for transaction use requires the close call to be called in finally block which provides for a stronger guarantee. 2. You mention in the description regarding test cases. Since you have done the majority of the work already, would you mind adding a few test cases to it to assert its workings? Apart from validating that the framework works well, these test cases will also help preserve the correctness from inadvertent changes that may indirectly impact its functioning. Thanks, Arvind - Arvind On 2012-01-18 00:05:21, Peter Newcomb wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3516/ > ----------------------------------------------------------- > > (Updated 2012-01-18 00:05:21) > > > 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. > > All that said, it wouldn't be a bad idea to create unit tests around these, > and ideally reusable test classes to test the basic use cases for any Channel > implementation or client. > > > Thanks, > > Peter > >
