Mailing-List: contact flume-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: flume-dev@incubator.apache.org
Content-Type: multipart/alternative;
	boundary="===============4944854071263442968=="
MIME-Version: 1.0
Subject: Re: Review Request: FLUME-936 MemoryChannel is not thread safe
From: "Juhani Connolly" <juhanic@gmail.com>
To: "Juhani Connolly" <juhanic@gmail.com>,
	"Arvind Prabhakar" <arvind@apache.org>,
	"Prasad Mujumdar" <prasadm@cloudera.com>,
	"Flume" <flume-dev@incubator.apache.org>
Date: Thu, 02 Feb 2012 00:42:37 -0000
Message-ID: <20120202004237.22953.88644@reviews.apache.org>
In-Reply-To: <20120201095519.20134.21724@reviews.apache.org>
References: <20120201095519.20134.21724@reviews.apache.org>

--===============4944854071263442968==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


-----------------------------------------------------------
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 redu=
cing 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 t=
hat 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 buff=
er of transactionCapacity. Another would be to implement the queue of queue=
s 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/BasicChannelSemantic=
s.java PRE-CREATION =

  flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSema=
ntics.java PRE-CREATION =

  flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java PR=
E-CREATION =

  flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java d=
379b64 =

  flume-ng-core/src/test/java/org/apache/flume/channel/TestFanoutChannel.ja=
va ada9a72 =

  flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.ja=
va b44030e =

  flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelCon=
currency.java PRE-CREATION =

  flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTra=
nsaction.java d18045b =

  flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java 6=
acbbd5 =


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 tr=
ansaction.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


--===============4944854071263442968==--