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 yuzhihong@gmail.com
 designates 209.85.192.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAAha9a3YyaNbYo9YVy1mLGGsVU2e_X_gZ36NbBaHAR31t5QHdA@mail.gmail.com>
References: 
 <CALte62zQoKu7yU5COFD4oqn62uMEU4pq4fz7K9ot7Q=dirFE6A@mail.gmail.com>
	<CALte62xWM9UmBn22FLcmayh4-yStMiSjfOhh-mUOisOJjYqeaw@mail.gmail.com>
	<CAAha9a3YyaNbYo9YVy1mLGGsVU2e_X_gZ36NbBaHAR31t5QHdA@mail.gmail.com>
Date: Mon, 2 Dec 2013 13:13:10 -0800
Message-ID: 
 <CALte62wTyHmfzMrQcP65RFP=3Shx79Ov9Ewbfta1k6+LgSFLAw@mail.gmail.com>
Subject: Re: Testing the fix for race condition between Compaction and
 StoreScanner.init
From: Ted Yu <yuzhihong@gmail.com>
To: "dev@hbase.apache.org" <dev@hbase.apache.org>
Content-Type: multipart/alternative; boundary=047d7b6dc31ead3c6404ec93a599

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

bq.  Would it be possible just to have it focus on just a region/store
level instead?

TestStoreScanner doesn't start minicluster.

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.


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
>

--047d7b6dc31ead3c6404ec93a599--