Flume
  1. Flume
  2. FLUME-936

MemoryChannel is not thread safe

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: NG alpha 2
    • Fix Version/s: v1.1.0
    • Component/s: Channel
    • Labels:
      None
    • Release Note:
      This depends on Flume-935 and should not be committed before that

      Description

      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.

      1. FLUME-936-3.patch
        39 kB
        Juhani Connolly
      2. FLUME-936-4.patch
        39 kB
        Juhani Connolly
      3. FLUME-936-unittest.patch
        3 kB
        Juhani Connolly

        Issue Links

          Activity

          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          Hudson added a comment -

          Integrated in flume-728 #115 (See https://builds.apache.org/job/flume-728/115/)
          FLUME-936: MemoryChannel is not thread safe
          (Juhani Connolly via Prasad Mujumdar) (Revision 1293084)

          Result = FAILURE
          prasadm : http://svn.apache.org/viewvc/?view=rev&rev=1293084
          Files :

          • /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java
          • /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java
          • /incubator/flume/branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java
          Show
          Hudson added a comment - Integrated in flume-728 #115 (See https://builds.apache.org/job/flume-728/115/ ) FLUME-936 : MemoryChannel is not thread safe (Juhani Connolly via Prasad Mujumdar) (Revision 1293084) Result = FAILURE prasadm : http://svn.apache.org/viewvc/?view=rev&rev=1293084 Files : /incubator/flume/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java /incubator/flume/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java /incubator/flume/branches/flume-728/flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java
          Hide
          Prasad Mujumdar added a comment -

          Patch committed to Flume 1.x codeline.
          Thanks Juhani !

          Show
          Prasad Mujumdar added a comment - Patch committed to Flume 1.x codeline. Thanks Juhani !
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          Juhani Connolly added a comment -

          Forgot to grant license

          Show
          Juhani Connolly added a comment - Forgot to grant license
          Hide
          Juhani Connolly added a comment -

          936-2 wasn't patching correctly against the latest flume-728 so here's an updated patchfile now that 935 is available.
          Tests pass, but there is some weirdness going on with rat failing on licenses because it tries to examine the surefire reports...

          Show
          Juhani Connolly added a comment - 936-2 wasn't patching correctly against the latest flume-728 so here's an updated patchfile now that 935 is available. Tests pass, but there is some weirdness going on with rat failing on licenses because it tries to examine the surefire reports...
          Hide
          Juhani Connolly added a comment -

          Patch not intended for inclusion until FLUME-935 has been committed. Additional fixes are necessary if FLUME-935 changes from its state in revision 5 of https://reviews.apache.org/r/3516/diff/#index_header

          Show
          Juhani Connolly added a comment - Patch not intended for inclusion until FLUME-935 has been committed. Additional fixes are necessary if FLUME-935 changes from its state in revision 5 of https://reviews.apache.org/r/3516/diff/#index_header
          Hide
          jiraposter@reviews.apache.org added a comment -

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

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

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/ ----------------------------------------------------------- (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. 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 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
          Hide
          Juhani Connolly added a comment -

          Prasad: still waiting on 935 to get committed... I looked at it yesterday and it looks good to me.
          However, it does lose the reentrant transaction semantics(which in my opinion makes sense), so I will need to disable the reentrant tests in this patch, I hope that is not a problem.
          I will submit the patch as soon as 935 has been committed

          Show
          Juhani Connolly added a comment - Prasad: still waiting on 935 to get committed... I looked at it yesterday and it looks good to me. However, it does lose the reentrant transaction semantics(which in my opinion makes sense), so I will need to disable the reentrant tests in this patch, I hope that is not a problem. I will submit the patch as soon as 935 has been committed
          Hide
          Prasad Mujumdar added a comment -

          @Juhani, please attach the latest patch to the Jira, make sure to grant license to Apache. I will go ahead and commit it.

          Show
          Prasad Mujumdar added a comment - @Juhani, please attach the latest patch to the Jira, make sure to grant license to Apache. I will go ahead and commit it.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          Juhani Connolly added a comment -

          Great! I'll keep an eye out for that

          Show
          Juhani Connolly added a comment - Great! I'll keep an eye out for that
          Hide
          Peter Newcomb added a comment -

          Sorry about the delay, but I'm just finishing up the unit tests around FLUME-935. Hope to post a new patch later this evening.

          Show
          Peter Newcomb added a comment - Sorry about the delay, but I'm just finishing up the unit tests around FLUME-935 . Hope to post a new patch later this evening.
          Hide
          Juhani Connolly added a comment -

          Totally forgot about 935 still not being committed. Hopefully the outstanding issues in that get fixed soon and I'll fix this against that once that happens.

          Show
          Juhani Connolly added a comment - Totally forgot about 935 still not being committed. Hopefully the outstanding issues in that get fixed soon and I'll fix this against that once that happens.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/ ----------------------------------------------------------- (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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/ ----------------------------------------------------------- (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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/ ----------------------------------------------------------- (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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          Juhani Connolly added a comment -

          Blocking on 935 makes total sense to me, and as I mentioned there, there was still a couple of things I was concerned in in that. Once that is settled, I will fix up any details left. I'll put together another unit test today.

          I'm also including in the fixes I made to flume-889 before(that issue can be cancelled as far as I'm concerned), the ones that prevented data loss on reconfiguration

          Show
          Juhani Connolly added a comment - Blocking on 935 makes total sense to me, and as I mentioned there, there was still a couple of things I was concerned in in that. Once that is settled, I will fix up any details left. I'll put together another unit test today. I'm also including in the fixes I made to flume-889 before(that issue can be cancelled as far as I'm concerned), the ones that prevented data loss on reconfiguration
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3704/ ----------------------------------------------------------- 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
          Hide
          Juhani Connolly added a comment -

          Yeah I can take a crack at it, might take a couple days due to other current priorities.

          Show
          Juhani Connolly added a comment - Yeah I can take a crack at it, might take a couple days due to other current priorities.
          Hide
          Prasad Mujumdar added a comment -

          The original implementation was addressing a single pushing events into the channel. Adding events directly into the shared queue with multiple sources would make it unnecessarily complicated. Keeping the new events local to the transaction make it much simpler.
          The transactions are already thread local and has local event queues (used for undo), though I guess using the common threadlocal transaction would be better ..

          Juhani, if you are planning to fix it, please go ahead. Otherwise I can pick it up.

          Show
          Prasad Mujumdar added a comment - The original implementation was addressing a single pushing events into the channel. Adding events directly into the shared queue with multiple sources would make it unnecessarily complicated. Keeping the new events local to the transaction make it much simpler. The transactions are already thread local and has local event queues (used for undo), though I guess using the common threadlocal transaction would be better .. Juhani, if you are planning to fix it, please go ahead. Otherwise I can pick it up.
          Hide
          Peter Newcomb added a comment -

          In case it's useful, take a look at FLUME-935, which is my attempt to provide a abstract implementation of Channel with threadlocal Transaction objects.

          Show
          Peter Newcomb added a comment - In case it's useful, take a look at FLUME-935 , which is my attempt to provide a abstract implementation of Channel with threadlocal Transaction objects.
          Hide
          E. Sammer added a comment -

          Completely agree, Juhani. Do you want to to take a crack at implementing outstanding transactions as threadlocals or something like that?

          Show
          E. Sammer added a comment - Completely agree, Juhani. Do you want to to take a crack at implementing outstanding transactions as threadlocals or something like that?
          Hide
          Juhani Connolly added a comment -

          The unit test recreates the following order;

          t1: begin transaction
          t1: put
          t2: begin transaction
          t2: put
          t1: rollback transaction <- Problem happens here as undoPut gets called, and removes the queue entry placed and stamped by t2
          t2: commit transaction <- As things are right now, this is actually committing the Event that was supposed to have been rolled back
          t1+t2: close transaction

          Overall it seems pretty hairy. One solution I can think of is to keep uncommitted entries in the thread-separate transaction items rather than keeping them in a shared queue

          Show
          Juhani Connolly added a comment - The unit test recreates the following order; t1: begin transaction t1: put t2: begin transaction t2: put t1: rollback transaction <- Problem happens here as undoPut gets called, and removes the queue entry placed and stamped by t2 t2: commit transaction <- As things are right now, this is actually committing the Event that was supposed to have been rolled back t1+t2: close transaction Overall it seems pretty hairy. One solution I can think of is to keep uncommitted entries in the thread-separate transaction items rather than keeping them in a shared queue

            People

            • Assignee:
              Juhani Connolly
              Reporter:
              Juhani Connolly
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development