hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: Add ability to have multiple Masters in LocalHBaseCluster for test writing
Date Fri, 01 Oct 2010 21:44:53 GMT

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


Mime
View raw message