Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: pass (athena.apache.org: domain of jon@cloudera.com designates
 209.85.220.177 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CALte62xVwKtvz-OYJVsvTQgtChdQqYs_jV-7JE0213p=dPEv_g@mail.gmail.com>
References: 
 <CALte62zQoKu7yU5COFD4oqn62uMEU4pq4fz7K9ot7Q=dirFE6A@mail.gmail.com>
 <CALte62xWM9UmBn22FLcmayh4-yStMiSjfOhh-mUOisOJjYqeaw@mail.gmail.com>
 <CAAha9a3YyaNbYo9YVy1mLGGsVU2e_X_gZ36NbBaHAR31t5QHdA@mail.gmail.com>
 <CALte62wTyHmfzMrQcP65RFP=3Shx79Ov9Ewbfta1k6+LgSFLAw@mail.gmail.com>
 <CAAha9a0nGy716JrtbbgeX5r6VB+UbHox8BcR7ENeVTiW=NRfrA@mail.gmail.com>
 <CALte62xVwKtvz-OYJVsvTQgtChdQqYs_jV-7JE0213p=dPEv_g@mail.gmail.com>
From: Jonathan Hsieh <jon@cloudera.com>
Date: Mon, 2 Dec 2013 21:06:27 -0800
Message-ID: 
 <CAAha9a3ndCSRJoLWjiODcURxxCm14XsRZCF-XrX--8N6D8opxA@mail.gmail.com>
Subject: Re: Testing the fix for race condition between Compaction and
 StoreScanner.init
To: "dev@hbase.apache.org" <dev@hbase.apache.org>
Content-Type: multipart/alternative; boundary=047d7b6dc6ee786e5104ec9a43aa

--047d7b6dc6ee786e5104ec9a43aa
Content-Type: text/plain; charset=ISO-8859-1

On Mon, Dec 2, 2013 at 7:34 PM, Ted Yu <yuzhihong@gmail.com> wrote:

> bq. The lastest submitted trunk patch
>
> The proposal here wouldn't use minicluster.
>
> Great!


> Cheers
>
>
> On Mon, Dec 2, 2013 at 7:31 PM, Jonathan Hsieh <jon@cloudera.com> wrote:
>
> > On Mon, Dec 2, 2013 at 1:13 PM, Ted Yu <yuzhihong@gmail.com> wrote:
> >
> > > bq.  Would it be possible just to have it focus on just a region/store
> > > level instead?
> > >
> > > TestStoreScanner doesn't start minicluster.
> > >
> > >
> > The lastest submitted trunk patch most certainly did.
> >
> > I think you can just create a standalone region with data with hooks for
> > testing when creating the store scanner
> >
> > +  /*
> > +   *  Verifies that there is no race condition between StoreScanner
> > construction and compaction.
> > +   *  This is done through 3 injection points:
> > +   *  1. before seek operation in StoreScanner ctor
> > +   *  2. after seek operation in StoreScanner ctor
> > +   *  3. after compaction completion
> > +   */
> > +  public void testCompactionRaceCondition() throws Exception {
> > +    HBaseTestingUtility util = new HBaseTestingUtility();
> > +    util.startMiniCluster(1);
> > +    byte[] t = Bytes.toBytes("tbl"), cf = Bytes.toBytes("cf");
> > +
> >
> > bq. Can you add enough info so that we don't need to consult the jira to
> > > figure out what the unit test is testing?
> > >
> > > Sure. I will add comments to the new test.
> > >
> > Looking forwards to it.
> >
> >
> > > Cheers
> > >
> > >
> > > On Mon, Dec 2, 2013 at 11:06 AM, Jonathan Hsieh <jon@cloudera.com>
> > wrote:
> > >
> > > > Part of the reason the injection stuff was "needed" was because the
> > test
> > > > crafted data using the higher level minicluster.  Would it be
> possible
> > > just
> > > > to have it focus on just a region/store level instead?  Along with
> the
> > > > focused unit test could a separate easy-to-read easy-to-maintain
> system
> > > > test be written as as well that compacts and starts scanners and
> > verifies
> > > > correctness.
> > > >
> > > > At this level, the code does not explain what the potential race we
> are
> > > > checking against is (other than something between storescanner
> > > construction
> > > > and compaction).  Can you add enough info so that we don't need to
> > > consult
> > > > the jira to figure out what the unit test is testing?
> > > >
> > > > Part of the issue is that the current patch and code attached doesn't
> > > make
> > > > it clear what is being verified.  There are no obvious assertions
> about
> > > > what is being checked (its tucked away in the
> > > > StoreScannercompactionRaceCondition InjectionHandler, at the least
> the
> > > > assertion should be at the testXxx method level.)
> > > >
> > > > I think the current test has hygiene issues as well (if the assert
> > > fails, I
> > > > think we leave a minicluster up?)
> > > >
> > > > Jon.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > On Mon, Dec 2, 2013 at 9:54 AM, Ted Yu <yuzhihong@gmail.com> wrote:
> > > >
> > > > > Is the proposed approach below acceptable ?
> > > > >
> > > > > Cheers
> > > > >
> > > > >
> > > > > On Wed, Nov 20, 2013 at 9:48 AM, Ted Yu <yuzhihong@gmail.com>
> wrote:
> > > > >
> > > > > > As requested by Jonathan, I am posting the following approach for
> > > > testing
> > > > > > fix of HBASE-9949 :
> > > > > >
> > > > > >
> > > > > > 1. Introduce config parameter for StoreScanner implementation
> which
> > > > would
> > > > > > be used by HStore for creating scanner
> > > > > > 2. In TestStoreScanner, add StoreScanner implementation which
> > extends
> > > > > > StoreScanner and set the above config parameter to this class.
> > > > > > 3. Register custom ChangedReadersObserver through the following
> API
> > > of
> > > > > > HStore :
> > > > > >
> > > > > >   public void addChangedReaderObserver(ChangedReadersObserver o)
> {
> > > > > >
> > > > > > The BEFORE_SEEK hook would be activated before Store.getScanner()
> > is
> > > > > > called.
> > > > > > The AFTER_SEEK hook can be activated at the end of ctor of
> > > StoreScanner
> > > > > > wrapper created in #2
> > > > > > The custom ChangedReadersObserver would activate the
> > COMPACT_COMPLETE
> > > > > hook.
> > > > > >
> > > > > > Comments are welcome.
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > // Jonathan Hsieh (shay)
> > > > // Software Engineer, Cloudera
> > > > // jon@cloudera.com
> > > >
> > >
> >
> >
> >
> > --
> > // Jonathan Hsieh (shay)
> > // Software Engineer, Cloudera
> > // jon@cloudera.com
> >
>


-- 
// Jonathan Hsieh (shay)
// Software Engineer, Cloudera
// jon@cloudera.com

--047d7b6dc6ee786e5104ec9a43aa--