----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/#review4871 -----------------------------------------------------------
Ship it! Looks good to me. Thanks for addressing all the issues and additional tests ! A minor suggestion, the commits/rollback are updating the putList and takeList under the queueLock. These can be moved outside the synchronized block to reduce the lock contention. Not important, you can log a separate jira later. - Prasad On 2012-02-06 01:37:09, Juhani Connolly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3704/ > ----------------------------------------------------------- > > (Updated 2012-02-06 01:37:09) > > > Review request for Flume. > > > Summary > ------- > > This is an initial go at fixing the threading issues with memory channel. > > It uses the preliminary work on FLUME-935 and I have included the code from > that. > > The tagging of the events became unnecessary so I dropped that. One thing > that concerns me slightly is how to deal with not having enough space in the > queue to rollback failed takes. One method would be to keep a minimum buffer > of transactionCapacity. Another would be to implement the queue of queues as > suggested in FLUME-889 > > Anyway, just putting up this early version to see what people think > > > This addresses bug FLUME-936. > https://issues.apache.org/jira/browse/FLUME-936 > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/channel/BasicChannelSemantics.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSemantics.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > d379b64 > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java > b44030e > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java > d18045b > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java > 46e42e3 > flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java > 9e465e1 > > Diff: https://reviews.apache.org/r/3704/diff > > > Testing > ------- > > The original tests pass, though I had to take out the state checks because of > the changes to semantics from the flume-935 code. I also had to add a > transaction.close statement where semantics were not properly being followed > I have to retrofit my new concurrency test since without the tagged events it > cannot fail without checking that the content is correct. I'll put that up > asap, just wanted to get some eyes on this before I head out. > > > Thanks, > > Juhani > >
