> On 2012-01-31 01:36:50, Arvind Prabhakar wrote: > > 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
I'll think about (1) some more (I think I had a reason to do it that way), and will either do as you suggest or document why I did it that way (in which case we can discuss it further). I'd be happy to do (2), and based on other testing I've done have already made a couple of minor fixes/tweaks to the patch anyway. Thanks for the review! -peter - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3516/#review4697 ----------------------------------------------------------- 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 > >
