[ 
https://issues.apache.org/jira/browse/FLUME-936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13197717#comment-13197717
 ] 

[email protected] commented on FLUME-936:
-----------------------------------------------------


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3704/
-----------------------------------------------------------

(Updated 2012-02-01 09:47:28.609110)


Review request for Flume.


Changes
-------

Too much contention for the locks in the original version... Some stuff could 
have the lock for a full duration of BlockingDeque.offer() and it was starving 
other threads.

Should be good now. If someone has time, I'd appreciate if they can have a long 
hard think over possible failure patterns. It is important to note that 
rollback can fail and will throw a ChannelException if there is no space to 
restore commits to. One possible way around this is to maintain an invariant 
across all threads that consists of queue.remainingCapacity() >= 
sumallthreads(takeList.size())      Feedback on that would be appreciated

I added in some unit tests to confirm the correct behavior of the capacities. 
Also added another highly parallel unit test to TestMemoryChannelConcurrency.
Modified existing unit tests where necessary to adjust configurations where 
transactioncapacity needed to be set.

Fixed coding guidelines stuff and added apache license.
Fixed  a couple of small bugs.


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 (updated)
-----

  
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


                
> MemoryChannel is not thread safe
> --------------------------------
>
>                 Key: FLUME-936
>                 URL: https://issues.apache.org/jira/browse/FLUME-936
>             Project: Flume
>          Issue Type: Bug
>          Components: Channel
>    Affects Versions: NG alpha 2
>            Reporter: Juhani Connolly
>            Assignee: Juhani Connolly
>             Fix For: v1.1.0
>
>         Attachments: FLUME-936-unittest.patch
>
>
> The memory channel isn't thread safe as a couple of parallel transactions can 
> commit/rollback each others entries if called in the wrong order.
> I'm attaching a unit test I made that demonstrates it using a cyclicbarrier 
> to force the event order that causes the precondition to fail.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to