hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Purtell <apurt...@apache.org>
Subject Re: Adding frameworks and coding for testabilitywas [Re: Testing the fix for race condition between Compaction and StoreScanner.init]
Date Sat, 23 Nov 2013 18:51:04 GMT
Personally I would try to mock before adding fault injection framework.
(Guilty of doing that in a recent patch-in-progress, but I have come to my
senses in time.) No objection to fault injection frameworks per se. Using
HDFS as an example again, please correct me if I'm mistaken, there was an
AOP fault injection framework once but it is currently disabled, a victim
of the migration from Ant to Maven, and possibly will be removed. The
trouble with testing frameworks is the added debt they accumulate over
time, like everything else. If we commit to adding one, and also use it as
much as possible, that would be fine with me. Either way, we should
definitely discuss the new/proposed framework and commit it on its own
JIRA. I'm concerned how one got through the back door on HBASE-9949.


On Fri, Nov 22, 2013 at 3:06 PM, Todd Lipcon <todd@cloudera.com> wrote:

> I haven't seen the particular "framework" in question, but I do think
> there's a continuum of reasonable choices here, ranging from full DI and
> using mocks/spys, to using AspectJ for AOP, to dding specific injections
> into the code at key points. I've experimented with probably the full range
> of options, and what I found to work best in HDFS was the addition of
> "FaultInjector" interfaces for each area of the code. For example:
>
> https://github.com/apache/hadoop-common/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CheckpointFaultInjector.java
> example usage:
>
> https://github.com/apache/hadoop-common/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java#L126
>
> I see Jon's point that this litters the code with explicit injection
> points. However, I disagree that that's a bad thing. Seeing a call to
> FaultInjector.foo() in the code makes a future developer more wary of where
> they need to be careful -- and ensures that, if they change the design of
> that area of the code, that they need to consider the same faults as the
> original. Additionally, contrary to the DI approach (which typically
> involves splitting classes into bunches of smaller units), the changes for
> this type of fault injection are well-contained: they add a line here or
> there, but they don't affect the higher-level design of the code, so you're
> free to code in a way that is more understandable instead of ending up with
> CheckpointDownloadStrategyFactoryFactories or something.
>
> Just one guy's opinion :)
>
> -Todd
>
>
>
>
>
> On Fri, Nov 22, 2013 at 12:54 PM, Stack <stack@duboce.net> wrote:
>
> > On Wed, Nov 20, 2013 at 3:27 PM, Jonathan Hsieh <jon@cloudera.com>
> 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.
> > >
> > >
> > How did such a mess even get committed?
> > St.Ack
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>



-- 
Best regards,

   - Andy

Problems worthy of attack prove their worth by hitting back. - Piet Hein
(via Tom White)

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