+1 on improving on testability / DI side of things.
On Wed, Nov 20, 2013 at 3:27 PM, Jonathan Hsieh <[email protected]> wrote: > tl;dr if we are going to add new frameworks, please make them a separate > patch. another attempt to bring up coding for testability/dep injection > again. > > I've changed the subject because I really meant this to be a bigger > discussion about setting up our code to be more testable and using > conventions that allow us to do dependency injection (ala guava or in a way > we can use mockito) or deciding to to include this new-to-me > InjectionHandler frameworks from the 89-fb branch. Porting HBASE-9949's > testing code depended this Injector infrastructure that I found > distasteful. > > This particular example smells funny and looked like another stovepiped > framework to me -- > 1) It added references and objects to our core code instead of having it > only present for our tests. > 2) This made the core code more cumbersome to read. > 3) Initial version used a brittle convention. Initially, the injection > point was identified by passing empty object array of particular size (0 > size mean option 0, size 1 meant injection point 1, etc). Later changed to > a enum, As used in the 89-fb branch we have a new global pool of unrelated > enum/constant values that seemed brittle for further extension. > > <soapbox>Generally tests should be able to setup custom objects with > constructors so we can use mockito or something similar in unit tests. If > done well we can instead of adding runtime injector objects to production > code. I'm making a plea to spend sometime tightening the code up and to > conveying more design in design. </soapbox> > > Recently been spending time in the hlog area and will likely personally > start there improving comments/names, conveying design intent, and > hopefully improving it. ;) > > Jon. > > 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] >
