hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gray" <jg...@apache.org>
Subject Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing
Date Wed, 06 Oct 2010 01:08:14 GMT


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 65
> > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line65>
> >
> >     This needs to be data member?  Its not just used once locally?

I copied from same way it's done for RS.  It is used in constructor and then also in addMaster()
method which is public and can be called by a client after initialization.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 141
> > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line141>
> >
> >     Why do this in constructor?  Why not declare and allocate at same time?

Again, copied from how it's done for RSs.  Can move it into the declaration for both.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 224
> > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line224>
> >
> >     No biggie but I like the previous syntax

Me too.  I was smacked two years ago doing one-line ifs and such, told that HBase form was
to always have braces.  I had my IDE setup to not allow the former but have changed it now
to permit one liners like this (when I hit code cleanup, it did this automatically).  Will
revert.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 267
> > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line267>
> >
> >     Can you read this from zk instead?

Yes, but I don't think that would be very useful for unit tests since you'd be relying on
correct behavior/interaction of masters w/ ZK.  The way this is done is super simple... when
a master becomes active, it flips a boolean.  My thought was to try and make this as fool
proof as possible.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 217
> > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line217>
> >
> >     No need for this comment.  CHeck the javadoc on this method.  It used to be
detailed steps IIRC.  The detail no longer is correct.

Revamped all comments.  Check next diff.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 322
> > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line322>
> >
> >     Does this if span too much?  Could you test stopped before calling initialization?
 loop already does stopped?

The if is immediately after we break out of blocking to become the active master.  This check
is for the case where we are backup master but then we are being shutdown.  We get interrupted
from blocking on becoming master, then we do this check.  What is inside the if{} is the stuff
we don't want to run (only run if you are actually active master).  The stuff below the if
is the stuff that is run in both cases of a master shutdown (backup master being shutdown,
or active master being shutdown in master or cluster shutdown).

Not the prettiest, tried a couple ways, but this ended up being simplest.  Will add some comments.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 345
> > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line345>
> >
> >     What services are started in teh constructor now?

Changed comment.  It's services that are started whether backup or active master.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line
674
> > <http://review.cloudera.org/r/925/diff/5/?file=13278#file13278line674>
> >
> >     Do we lose messages here?  Important we not drop split, etc.  FYI, we need to
do some more thought around split message and master failover. See over in master where we
register servers on region server report for note.

Shouldn't be any different than before.  If we get IOE, we wouldn't have any messages returned
from the method.

Agree we need to more thought on this.  Why piggyback this on heartbeat?  Maybe direct RPC
and master can return something that says success?  If fail, then it'll retry from RS?


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, line
133
> > <http://review.cloudera.org/r/925/diff/5/?file=13283#file13283line133>
> >
> >     How about a test where there are RIT and master joins and assertion it does
the fixup?
> >     
> >     There is white space above the 'yay'.

That's the plan.


> On 2010-10-01 14:44:53, stack wrote:
> > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, line
86
> > <http://review.cloudera.org/r/925/diff/5/?file=13283#file13283line86>
> >
> >     Yeah, whats this about?

Will try and get rid of it all.  The test is just a first go.  I'd like to get this committed
because HMaster and HRS keep changing and I have to rebase the changes.

So I'm looking for a +1 to commit to get the changes in and then I'll continue working on
fancier tests with this stuff.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/925/#review1378
-----------------------------------------------------------


On 2010-10-01 10:00:03, Jonathan Gray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/925/
> -----------------------------------------------------------
> 
> (Updated 2010-10-01 10:00:03)
> 
> 
> Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.
> 
> 
> Summary
> -------
> 
> To really be able to unit test the new master properly, we need to be able to have multiple
masters running at once within a single logical cluster.
> 
> Also exposes methods to get the currently active master and can access each individually
in the same way that it's done for multiple RS.
> 
> This is mostly duplicating what we do for RS but for masters.
> 
> 
> This addresses bug HBASE-3053.
>     http://issues.apache.org/jira/browse/HBASE-3053
> 
> 
> Diffs
> -----
> 
>   trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java 1003327 
>   trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1003327 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1003327 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java 1003327

>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1003327

>   trunk/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 1003327 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 1003327

>   trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1003327 
>   trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 1003327 
>   trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java PRE-CREATION

> 
> Diff: http://review.cloudera.org/r/925/diff
> 
> 
> Testing
> -------
> 
> Untested but compiles.  This is first go at a patch.  I'm going to work on writing a
new unit test that uses this next.
> 
> 
> Thanks,
> 
> Jonathan
> 
>


Mime
View raw message