On Mon, Dec 2, 2013 at 7:34 PM, Ted Yu <[email protected]> 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 <[email protected]> wrote: > > > On Mon, Dec 2, 2013 at 1:13 PM, Ted Yu <[email protected]> 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 <[email protected]> > > 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 <[email protected]> wrote: > > > > > > > > > Is the proposed approach below acceptable ? > > > > > > > > > > Cheers > > > > > > > > > > > > > > > On Wed, Nov 20, 2013 at 9:48 AM, Ted Yu <[email protected]> > 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 > > > > // [email protected] > > > > > > > > > > > > > > > -- > > // Jonathan Hsieh (shay) > > // Software Engineer, Cloudera > > // [email protected] > > > -- // Jonathan Hsieh (shay) // Software Engineer, Cloudera // [email protected]
