Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: pass (nike.apache.org: domain of todd@cloudera.com designates
 209.85.215.51 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CADcMMgFw_cnsVMVOXZeY5fkkMyu9DpJtY+FfMFP8qNyYhSAZPg@mail.gmail.com>
References: 
 <CAAha9a3O42cH5p=TLhMJUyOVgspsPfr3wEXgcWdC2O8v4i0r1w@mail.gmail.com>
 <CADcMMgFw_cnsVMVOXZeY5fkkMyu9DpJtY+FfMFP8qNyYhSAZPg@mail.gmail.com>
From: Todd Lipcon <todd@cloudera.com>
Date: Fri, 22 Nov 2013 15:06:23 -0800
Message-ID: 
 <CADY20s5zHNCi9UpspFzBH3WWiuj8s-KnM5XW+t7kp8mCSFuU=A@mail.gmail.com>
Subject: Re: Adding frameworks and coding for testabilitywas [Re: Testing the
 fix for race condition between Compaction and StoreScanner.init]
To: dev <dev@hbase.apache.org>
Content-Type: multipart/alternative; boundary=001a11c343ba53137b04ebcc11e5

--001a11c343ba53137b04ebcc11e5
Content-Type: text/plain; charset=ISO-8859-1

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

--001a11c343ba53137b04ebcc11e5--