> On 2012-02-01 18:09:27, Prasad Mujumdar wrote: > > The changes overall look fine. A couple of comments, > > > > 1) It should be okay to bump up the queue capacity during before rollback > > start, to ensure sufficient space to return the events. > > > > 2) The commit() after put() and rollback() after take() are both > > adding elements to the channel's queue. But they are under different locks. > > That means concurrent source and sink could cause queue to reach capacity > > in the middle of commit/rollback and one of them would be partially > > complete. I guess its better to use same lock for both operations. > > > > > >
Totally right about 2) I've fixed that and will put the patch for that up now As far as 1) resizing the queue is pretty awkward. Anything we resize up for rollback would have to be resized back down at some time. If we want to guarantee that rollbacks succeed, I think it would be better just to reserve space in the main queue for the contents of every threads take queue, by maintaining queue.remainingCapacity() >= sumallthreads(takeList.size()) perhaps this should be a separate issue? - Juhani ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/#review4750 ----------------------------------------------------------- On 2012-02-01 09:55:19, Juhani Connolly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3704/ > ----------------------------------------------------------- > > (Updated 2012-02-01 09:55:19) > > > 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/TestFanoutChannel.java > ada9a72 > 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 > 6acbbd5 > > 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 > >
