You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Todd Lipcon <to...@cloudera.com> on 2010/09/14 03:36:30 UTC
Review Request: Refactor master and RS to conform to Guava service lifecycle
interface
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/829/
-----------------------------------------------------------
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
Re: Review Request: Refactor master and RS to conform to Guava
service lifecycle interface
Posted by Jay Booth <ja...@gmail.com>.
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, <st...@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
> >
> >
>
>
Re: Review Request: Refactor master and RS to conform to Guava service
lifecycle interface
Posted by st...@duboce.net.
-----------------------------------------------------------
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
>
>
Re: Review Request: Refactor master and RS to conform to Guava service
lifecycle interface
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/829/#review1173
-----------------------------------------------------------
I like this. Naming seems fine to me. We're making great strides in cleaning these things up, great stuff Todd.
I guess we're in need of standardizing the methods in HM and HRS like toString, get*Name, etc...
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/829/#comment4026>
The toString() is supposed to give a unique name for the instance, especially useful in unit tests with multiple masters and such. Not sure where all we're using getServerName but might make sense for these to be the same.
- Jonathan
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
>
>