hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Booth <jaybo...@gmail.com>
Subject Re: Review Request: Refactor master and RS to conform to Guava service lifecycle interface
Date Tue, 14 Sep 2010 06:21:30 GMT
If you guys are thinking dependency injection, using Guice, constructor
injection and final variables for dependencies has been a great formula for
me on some stuff recently.  I'm not a Spring expert but it was almost no
work and felt a lot safer than xml-wired setter injection.

On Tue, Sep 14, 2010 at 1:51 AM, <stack@duboce.net> wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/829/#review1176
> -----------------------------------------------------------
>
>
>
> src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
> <http://review.cloudera.org/r/829/#comment4028>
>
>    Didn't realize one of these lifecycle things in Guava but then there's a
> million different versions of Lifecycle I suppose (I'm pretty sure I've
> written a few myself in past life).  We could write our own but, its my
> guess it'd not differ from what we have here much.  Then again its missing
> Abort.
>
>    This kinda refactoring is a big improvement; we should go this way,
> yeah, as it takes us further down road already started where we're trying to
> have Servers in hbase look alike.  But I want us to be even more radical.  I
> want us to move up to something like Spring where implementing their
> container means we get a bunch of other facility for near free and where the
> wiring up of a regionserver or a master can be done from config with others
> able to provide alternate implementations if they'd like with just a config.
> change.  It'd encourage writing to Interfaces, etc. ( Well, maybe not
> Spring.  Its too heavy weight and that xml stuff makes me queasy.  There are
> lesser IofC containers such as pico or nano).
>
>
>
> src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
> <http://review.cloudera.org/r/829/#comment4029>
>
>    This is good
>
>
>
> src/main/java/org/apache/hadoop/hbase/master/HMaster.java
> <http://review.cloudera.org/r/829/#comment4030>
>
>    Thats a real pretty name (descriptive though I suppose)
>
>
>
> src/main/java/org/apache/hadoop/hbase/master/HMaster.java
> <http://review.cloudera.org/r/829/#comment4031>
>
>    We had a sort of convention for naming threads in hbase where host
> 'server' is prefix where host includes service type and port if needed...
> that kinda thing.  Previous we had a naming convention per implementator...
> was a mess.
>
>
>
> src/main/java/org/apache/hadoop/hbase/master/HMaster.java
> <http://review.cloudera.org/r/829/#comment4032>
>
>    I like having state in there.  I was also adding zk sessionid....
>
>
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
> <http://review.cloudera.org/r/829/#comment4033>
>
>    good
>
>
>
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
> <http://review.cloudera.org/r/829/#comment4034>
>
>    good
>
>
>
>
> src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java
> <http://review.cloudera.org/r/829/#comment4035>
>
>    Good.  I think I made this class originally.  It grew into a monster.
>
>
>
> src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java
> <http://review.cloudera.org/r/829/#comment4036>
>
>    Are we losing the threaddumping?  It was useful.
>
>
> - stack
>
>
> On 2010-09-13 18:36:29, Todd Lipcon wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://review.cloudera.org/r/829/
> > -----------------------------------------------------------
> >
> > (Updated 2010-09-13 18:36:29)
> >
> >
> > Review request for hbase, stack and Jonathan Gray.
> >
> >
> > Summary
> > -------
> >
> > This isn't really cleaned up, but wanted to gather opinions before I put
> in more effort. Does this kind of refactor seem good? Is the Guava "Service"
> class what we want to use or should we just write our own similar interface?
> >
> >
> > This addresses bug HBASE-2993.
> >     http://issues.apache.org/jira/browse/HBASE-2993
> >
> >
> > Diffs
> > -----
> >
> >   src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java bc0a62f
> >   src/main/java/org/apache/hadoop/hbase/master/HMaster.java bc21a1e
> >   src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java
> c675db9
> >   src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
> bba7b67
> >   src/main/java/org/apache/hadoop/hbase/service/ServiceUtils.java
> PRE-CREATION
> >
> src/main/java/org/apache/hadoop/hbase/service/ServiceWithMainThread.java
> PRE-CREATION
> >   src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 4a9f1c3
> >   src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java b7abb51
> >   src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 9cc1168
> >   src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 9407c1e
> >   src/test/java/org/apache/hadoop/hbase/TestHBaseTestingUtility.java
> 1cd88d3
> >   src/test/java/org/apache/hadoop/hbase/TestInfoServers.java daffe02
> >   src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java
> 6908111
> >
> src/test/java/org/apache/hadoop/hbase/master/TestKillingServersFromMaster.java
> f5fd243
> >
> src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java
> a172e2c
> >
> src/test/java/org/apache/hadoop/hbase/regionserver/DisabledTestRegionServerExit.java
> 5b8b464
> >
> src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
> 287f1fb
> >
> > Diff: http://review.cloudera.org/r/829/diff
> >
> >
> > Testing
> > -------
> >
> > Tried to run unit tests, but plenty failed. I think they're failing on
> trunk, too, though. Will do more testing and another round of review before
> I actually claim this is ready to commit.
> >
> >
> > Thanks,
> >
> > Todd
> >
> >
>
>

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