hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jonathan Hsieh <...@cloudera.com>
Subject Re: Testing the fix for race condition between Compaction and StoreScanner.init
Date Tue, 03 Dec 2013 03:31:09 GMT
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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message