You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@flume.apache.org by Juhani Connolly <ju...@gmail.com> on 2012/02/01 10:47:28 UTC

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

-----------------------------------------------------------
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

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Prasad Mujumdar <pr...@cloudera.com>.

-----------------------------------------------------------
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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

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

(Updated 2012-02-21 06:51:00.751898)


Review request for Flume.


Changes
-------

Rebased patch and tested with following:

 1005  git pull origin flume-728
 1009  git diff 44850b9989a4c13716cbc6 > FLUME-936-4.patch
 1010  git checkout origin/flume-728 -b temp
 1012  git apply FLUME-936-4.patch
 1014  mvn clean
 1015  mvn test

Results are fine

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] Apache Flume ...................................... SUCCESS [4.458s]
[INFO] Flume NG Core ..................................... SUCCESS [1:47.853s]
[INFO] Flume NG Sinks .................................... SUCCESS [0.096s]
[INFO] Flume NG HDFS Sink ................................ SUCCESS [14.040s]
[INFO] Flume NG IRC Sink ................................. SUCCESS [0.890s]
[INFO] Flume NG Channels ................................. SUCCESS [0.065s]
[INFO] Flume NG JDBC channel ............................. SUCCESS [28.434s]
[INFO] Flume NG Node ..................................... SUCCESS [31.250s]
[INFO] Flume NG file-based channel ....................... SUCCESS [1.793s]
[INFO] Flume NG distribution ............................. SUCCESS [0.754s]
[INFO] Flume legacy Sources .............................. SUCCESS [0.134s]
[INFO] Flume legacy Thrift Source ........................ SUCCESS [3.043s]
[INFO] Flume legacy Avro source .......................... SUCCESS [2.876s]
[INFO] Flume NG Clients .................................. SUCCESS [0.062s]
[INFO] Flume NG Log4j Appender ........................... SUCCESS [3.732s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3:19.980s
[INFO] Finished at: Tue Feb 21 15:49:08 JST 2012
[INFO] Final Memory: 83M/229M
[INFO] ------------------------------------------------------------------------


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/MemoryChannel.java 6a17f06 
  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 3014368 
  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

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.


> On 2012-02-21 06:40:54, Arvind Prabhakar wrote:
> > Thanks for the patch Juhani. Can you please rebase it and update the review? It is not applying cleanly for MemoryChannel.java. 
> >
> 
> Juhani Connolly wrote:
>     I just diffed patch-3 and patch-4(from my updated diff below) and they appear to be identical.
>     Can you paste the output of the failed apply attempt?

just to be totally sure, I did a fresh svn checkout/patch and it was fine.

 1032  mkdir flume
 1033  cd flume
 1034  svn checkout https://svn.apache.org/repos/asf/incubator/flume/branches/flume-728/
 1035  cd flume-728/
 1037  patch -p1 < ~/workspace/flume/FLUME-936-4.patch 
 1038  mvn test


- Juhani


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


On 2012-02-21 06:51:00, Juhani Connolly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3704/
> -----------------------------------------------------------
> 
> (Updated 2012-02-21 06:51:00)
> 
> 
> 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/MemoryChannel.java 6a17f06 
>   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 3014368 
>   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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Arvind Prabhakar <ar...@apache.org>.


> On 2012-02-21 06:40:54, Arvind Prabhakar wrote:
> > Thanks for the patch Juhani. Can you please rebase it and update the review? It is not applying cleanly for MemoryChannel.java. 
> >
> 
> Juhani Connolly wrote:
>     I just diffed patch-3 and patch-4(from my updated diff below) and they appear to be identical.
>     Can you paste the output of the failed apply attempt?
> 
> Juhani Connolly wrote:
>     just to be totally sure, I did a fresh svn checkout/patch and it was fine.
>     
>      1032  mkdir flume
>      1033  cd flume
>      1034  svn checkout https://svn.apache.org/repos/asf/incubator/flume/branches/flume-728/
>      1035  cd flume-728/
>      1037  patch -p1 < ~/workspace/flume/FLUME-936-4.patch 
>      1038  mvn test

Thanks Juhani. I probably did not apply the patch correctly or had some local changes at the time that may have caused conflict. Since I do not have the workspace now I cannot say for sure what the problem was. However, since the patch was committed, chances are there was no problem and it was a pilot error on my part. Thanks for making doubly sure of it though.


- Arvind


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


On 2012-02-21 06:51:00, Juhani Connolly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3704/
> -----------------------------------------------------------
> 
> (Updated 2012-02-21 06:51:00)
> 
> 
> 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/MemoryChannel.java 6a17f06 
>   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 3014368 
>   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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.


> On 2012-02-21 06:40:54, Arvind Prabhakar wrote:
> > Thanks for the patch Juhani. Can you please rebase it and update the review? It is not applying cleanly for MemoryChannel.java. 
> >

I just diffed patch-3 and patch-4(from my updated diff below) and they appear to be identical.
Can you paste the output of the failed apply attempt?


- Juhani


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


On 2012-02-21 06:51:00, Juhani Connolly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3704/
> -----------------------------------------------------------
> 
> (Updated 2012-02-21 06:51:00)
> 
> 
> 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/MemoryChannel.java 6a17f06 
>   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 3014368 
>   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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Arvind Prabhakar <ar...@apache.org>.

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


Thanks for the patch Juhani. Can you please rebase it and update the review? It is not applying cleanly for MemoryChannel.java. 


- Arvind


On 2012-02-20 05:09:19, Juhani Connolly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3704/
> -----------------------------------------------------------
> 
> (Updated 2012-02-20 05:09: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/MemoryChannel.java 6a17f06 
>   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 3014368 
>   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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

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

(Updated 2012-02-20 05:09:19.954344)


Review request for Flume.


Changes
-------

Uploaded FLUME-936-3.patch
There is no difference in this patch from the last one, just based on the latest flume-728 as the previous patch wasn't applying cleanly


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/MemoryChannel.java 6a17f06 
  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 3014368 
  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

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Arvind Prabhakar <ar...@apache.org>.

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


Juhani - can you please confirm if this change is the latest? I see this diff is named FLUME-936-2.patch while the one attached to the jira is FLUME-936-3.patch. 

Should I go ahead and review this or are you planning to update this diff?


- Arvind


On 2012-02-08 06:47:33, Juhani Connolly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3704/
> -----------------------------------------------------------
> 
> (Updated 2012-02-08 06:47:33)
> 
> 
> 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/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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

(Updated 2012-02-08 06:47:33.989223)

Review request for Flume.

Changes
-------

This is the diff that does not include the work from FLUME-935 that should be committed once that has gone through.

There was a concurrency bug in the unit test which I have now fixed.
I also improved concurrency by introducing a couple of semaphores so that threads could wait on data without having to block inside the synchronization mechanisms(the patch originally simply didn't block... Which for some usages would be rather user-unfriendly

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.

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/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
-------

Thanks,

Juhani

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

(Updated 2012-02-06 01:37:09.579940)

Review request for Flume.

Changes
-------

This is a final patch(unless someone runs into problems with it).
Updated for the latest 728 and I made the tests independent of each other to avoid problems with mvn's parallel testing.
Everything runs through the full test suite in mvn without errors, and the new tests include checking that no data is lost with multiple threads parallel put/taking.

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.

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/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
-------

Thanks,

Juhani

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

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

(Updated 2012-02-02 10:23:16.661600)


Review request for Flume.


Changes
-------

One more diff because I had only been running the ng-core tests. Some of the other tests also fail without adjusting the memory channel configuration(I just made the defaults larger rathar than adjusting many different tests)

Additionally, when running the tests via mvn as opposed to running them one at a time through my IDE, testConcurrentSinksAndSources fails with mismatched put and take counts. I cannot recreate this regardless of how many times I run the test on its own so I'm pretty sure that it is because of mvn concurrently running multiple tests at a time which are then interfering with one another, though if anyone has any other ideas I'm all ears.


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/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

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

(Updated 2012-02-02 02:27:14.322838)

Review request for Flume.

Changes
-------

I decided to address Prasad's issue with failing rollbacks by implementing my invariant idea.
I also added in a concurrent sink + source test, with a tight capacity(only 100), and 50 each of randomly committing/rollbacking sinks and sources.

One can note that in it, a failed commit for sources is not exception handled, so the thread would die and with that the test.
On the other hand, the source thread commits are inside a try/catch and can fail. If they do, they are just rolled back. In practice this means that barring agent failure, anything that has been succesfully committed to the memory channel shouldn't ever be lost.

I think that the code is pretty hard to understand... I have tried to annotate and document concurrency guards and invariants, if anyone can think of ways to make it easier to understand let me know.

Other than that, I believe this patch should hopefully be my final one, solving the outstanding issues. I will be sure to remove the 935 changes from it once I put it up to the JIRA

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.

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

Diff: https://reviews.apache.org/r/3704/diff

Testing
-------

Thanks,

Juhani

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

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

(Updated 2012-02-02 00:42:37.375992)


Review request for Flume.


Changes
-------

This addresses Prasad's concern regarding the queue getting full up by reducing to one lock


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

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.


> 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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Prasad Mujumdar <pr...@cloudera.com>.

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


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. 




- Prasad


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
> 
>

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

(Updated 2012-02-01 09:55:19.470045)

Review request for Flume.

Changes
-------

white-space... and forgetting to git add, hence the changeless diff

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.

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

Diff: https://reviews.apache.org/r/3704/diff

Testing
-------

Thanks,

Juhani

Re: Review Request: FLUME-936 MemoryChannel is not thread safe

Posted by Juhani Connolly <ju...@gmail.com>.

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

(Updated 2012-02-01 09:52:49.475183)


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