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: HBASE-3047: If new master crashes, restart is messy
Date Wed, 29 Sep 2010 00:49:50 GMT

This is an automatically generated e-mail. To reply, visit:

Overall this looks like a good improvement over what we had.  I'm still a little confused
about isRunningCluster (or isProperRunningCluster per comments).

Repeat from inline comments, but, is there ever a time a single region is deployed and we
don't want to trigger the failover codepath?

Isn't the case we're really protecting against here that the cluster was not shutdown properly
so the cluster status flag is up when it shouldn't be?

And does this handle case that cluster is killed quickly and then restarted again so the master
ephemeral node is actually still there?  Then the RS will have master node and cluster up
node and startup but potentially without a real master?


    Why is this an "implementation"?  Doesn't the HRI represent the actual connection object?
 I get that it's an implementation of HRI but normally that would be used in class names implementing?
 No biggie, should just be consistent and seems a weird name to me (I think I was referring
to this stuff as "connection" elsewhere in the class in method names/variable names)


    Is this really the exception we want to throw (commons.lang)?  Or this is just short-term


    yay thanks


    So case that we are adding for here (but just throwing exception for now) is master came
up, did not think it was fresh cluster (because cluster status flag in zk up? maybe note in
comments above?), but we determine the cluster was not running because ROOT and META are not
    What about case where other regions are assigned?  Should this check actually be whether
_any_ regions are assigned?  I think we discussed this, and I think looking for root/meta
covers most cases, but maybe add a TODO?
    Though, even in failover case, we'll need to handle ROOT/META not being properly assigned,
so if _any_ regions are assigned we would trigger failover, if no regions assigned we would
assume it actually is a cluster startup and go into the branch of code which currently throws
the exception.


    javadoc about what this method does to determine if it's running cluster


    So this method would be "proper running cluster"?
    Isn't it the case that if a single region is deployed anywhere we are not in startup,
we are failover?


    looks good

- Jonathan

On 2010-09-28 15:52:46, Jonathan Gray wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/915/
> -----------------------------------------------------------
> (Updated 2010-09-28 15:52:46)
> Review request for hbase, stack and Jonathan Gray.
> Summary
> -------
> This is patch from Stack, just putting up on rb.
> M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
>   Add test of case where HRegionInterface connection throws a
>   ConnectionException. Also tests two new verify root and meta 
>   locations added to CatalogTracker.
> M src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
>   Change order in which we start up trackers in ZK.  Also add blocking
>   until master is up to make it less likely we'll start before master
>   comes up, especially around the cluster start up situation.
> M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
>   Introduce new state on startup, the case where the cluster is
>   NOT a fresh startup and its NOT a cluster where all is fully
>   assigned.  The repair the master needs run to fixup this new
>   state is not yet done; we throw a NotImplementedException for
>   now.  TODO.  Added new isRunningCluster checker used figuring
>   what the cluster condition is when master is joining.  Not
>   comprehensive but good enough for now.
> M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
>   Javadoc.
>   Added new verifyRootRegionLocation and verifyMetaRegionLocation.
>   Needed to verify whats in zk is actually locations of catalog
>   regions.
> M src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
>   Add fact that the verifying method, getRegionInfo, can throw
>   ConnectException
> This addresses bug HBASE-3047.
>     http://issues.apache.org/jira/browse/HBASE-3047
> Diffs
> -----
>   trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1002359 
>   trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1002359 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1002359 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1002359

>   trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java 1002359

> Diff: http://review.cloudera.org/r/915/diff
> Testing
> -------
> Thanks,
> Jonathan

View raw message