----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/#review4706 -----------------------------------------------------------
Thanks for the patch Juhani. Here is my high-level feedback: 1. Since this depends upon FLUME-935, I have marked the issue blocked by that JIRA. I would rather check that in first than have the sources from there be part of the patch here. Accordingly, my feedback is limited to the sources that are not part of FLUME-935. 2. The concurrent tests that you have added are a good first step. However, I suggest adding another test that has at least 10 simulated sources and sinks with over a hundred events exchanging hands. While the current test asserts correctness over a known scenario, this new test will be able to chance upon failures that may otherwise go unnoticed. 3. For some reason, the indentation and whitespace is not looking right in the review. I suggest you update your IDE preferences to replace all tabs with spaces, and use a 2-space indent policy. Also, please remove any trailing whitespaces from the code anywhere. Personally, I use AnyEdit tool plugin on my Eclipse which allows the removal of trailing whitespaces on file save. Other tools would work great as well. Thanks - Arvind On 2012-01-31 07:46:56, Juhani Connolly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3704/ > ----------------------------------------------------------- > > (Updated 2012-01-31 07:46:56) > > > 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 > > 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 > >
