You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@hbase.apache.org by Jonathan Gray <jg...@apache.org> on 2010/10/01 19:00:03 UTC

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

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

(Updated 2010-10-01 10:00:03.133377)

Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

Changes
-------

Adds TestMasterFailover which passes! Starts cluster w/ 3 masters. Stops a backup master, then stops the active master, and verifies the other backup becomes the active master. Doesn't test w/ tables/data or regions-in-transition but the test framework improvements work.

A few things that were changed:

- can now close backup masters
- can now close active master w/o shutting down cluster
- regionserver reconnects to new master when fail over

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 (updated)
-----

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

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by st...@duboce.net.

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

Ship it!


Quick review.  Looks good.  Fix the below on commit.


trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment5095>

    isReady and isActive are not how to name booleans.  This is what the methods should be named.  I'd suggest activeMaster and started as names for these flags.


- stack


On 2010-10-06 14:35:17, Jonathan Gray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/925/
> -----------------------------------------------------------
> 
> (Updated 2010-10-06 14:35:17)
> 
> 
> 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 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 1004853 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 1004853 
>   trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1004853 
>   trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 1004853 
>   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
> 
>

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by Jonathan Gray <jg...@apache.org>.

(Updated 2010-10-06 14:35:17.785727)

Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

Changes
-------

Changes names of state variables/methods in HMaster to be more explicit. isActiveMaster and isInitialized now.

Also removes the last warning in HMaster related to using deprecated RemoteException unwrapping methods.

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 (updated)
-----

trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java 1004853
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 1004853
trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1004853
trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 1004853
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

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by Jonathan Gray <jg...@apache.org>.

(Updated 2010-10-06 14:14:23.202596)

Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

Changes
-------

More cleanup of comments. Removed last remnants of numbering. Undid some of the code cleanup my IDE did around single line ifs.

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 (updated)
-----

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

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by Jonathan Gray <jg...@apache.org>.

(Updated 2010-10-06 12:45:19.045732)

Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

Changes
-------

Removes all the sleeps from the test. Adds a new 'isReady' flag into HMaster so we know when it has finished doing initialization. There is now an 'isActive()' and 'isReady()' which can be used to determine when the cluster/failover has been completed. Exposes a method in MiniHBaseCluster called waitForActiveAndReadyMaster() which blocks until one of the masters becomes active and finish initializing. That method can be interrupted and will also return false if there are no masters running at all.

Cleans up a little logging and comments as well.

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 (updated)
-----

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

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by Jonathan Gray <jg...@apache.org>.

(Updated 2010-10-05 18:15:55.818277)

Review request for hbase, stack, Karthik Ranganathan, and Kannan Muthukkaruppan.

Changes
-------

Incorporates changes from stack review.

Test itself is the same. I'd like to get this committed and then will work on more tests as a separate JIRA. The changes to HMaster and HRS are hard to keep up to date with trunk and I'll be doing more tests w/ this stuff over the next two weeks.

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 (updated)
-----

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

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by Jonathan Gray <jg...@apache.org>.


> 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
> 
>

Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing

Posted by st...@duboce.net.

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


This is great. Follows the ugly pattern that was in place for RSs.  Just some comments below... see what you think.


trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/925/#comment4564>

    This needs to be data member?  Its not just used once locally?



trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/925/#comment4565>

    Why do this in constructor?  Why not declare and allocate at same time?



trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/925/#comment4566>

    No biggie but I like the previous syntax



trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/925/#comment4567>

    Can you read this from zk instead?



trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment4568>

    Go ahead and remove it.  It used to be used figuring if fresh startup but now its just logged to help debugging what state was like on master startup.



trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment4569>

    No need for this comment.  CHeck the javadoc on this method.  It used to be detailed steps IIRC.  The detail no longer is correct.



trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment4570>

    Yeah, this 3. is a bit odd in here w/o its 1. and 2.



trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment4571>

    Does this if span too much?  Could you test stopped before calling initialization?  loop already does stopped?



trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/925/#comment4572>

    What services are started in teh constructor now?



trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/925/#comment4575>

    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.



trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
<http://review.cloudera.org/r/925/#comment4576>

    Yeah, whats this about?



trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
<http://review.cloudera.org/r/925/#comment4577>

    How about a test where there are RIT and master joins and assertion it does the fixup?
    
    There is white space above the 'yay'.


- stack


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
> 
>