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-3112 Enable and disable of table needs a bit of loving in new master
Date Mon, 08 Nov 2010 20:13:11 GMT

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


Looks pretty good.  Should be much improved.

What about client-side semantics and checks?  I thought there was going to be some change
there?  An async trigger and then a fast way to see if it's done or not?  Should be clear
on javadoc since enable/disable is always thing we get questions on.

I think most of my issues with patch is that it adds huge amount of new code into already
big AssignmentManager class.  Should move these classes out and not sure if we should have
these methods which just touch ZK.  I think having all logic about the state transitions,
usage of ZK, etc more explicitly controlled in the handlers themselves might be more clear
to follow how this works.  Maybe not but was a little hard to follow (that, for example, we
don't have enabling/disabling states in memory... where we were talking about in-memory state
vs zk states, etc).

Also, ZKTable can be simplified (and made more future proof) with an enum.

Now, for failover, I know we said we would punt to 0.92 on handling of this case, but we should
at least make it so we don't get into broken state if we have a master failover.  What will
happen if we are in disabling up in zk but not all closes have been done?

Lastly... I think we definitely need some unit tests on this stuff.  TestMasterFailover does
a little but not really relevant to these changes (only deals with regions already RIT). 
TestRollingRestart does an enable/disable of table.  But none of this stuff takes into account
failure of stuff in handlers, failure of RS, etc... Don't need to hold up this patch on unit
tests if it's working in cluster testing on large tables, but it's kind of thing that would
be good to test at some point.


trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5966>

    I thought we had this method somewhere already?



trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5967>

    nevermind, you just moved it :)



trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5968>

    Whitespace, and should make note that this is checking ZK not in-memory state?



trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5969>

    Are these methods necessary?  Seems like unnecessary stuff in AssignmentManager.  The
Handlers can just use the ZK methods directly?  (keep the AM methods as check/set of it's
internal state?)



trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java
<http://review.cloudera.org/r/1187/#comment5970>

    This is for repeated runs of enable?  Should we log if this actually removes regions (table
has X total regions, already Y online, assigning Z still offline)?
    
    It's okay that this operation is not done under any locks?  Couldn't regions be coming
online/offline concurrent with this operation?



trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java
<http://review.cloudera.org/r/1187/#comment5972>

    whitespace



trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java
<http://review.cloudera.org/r/1187/#comment5973>

    enum?



trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java
<http://review.cloudera.org/r/1187/#comment5975>

    yeah, instead of all these permutations on isEnabledOrDisabling and what not, should just
use an enum and pass that in as argument to method which checks node state.



trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
<http://review.cloudera.org/r/1187/#comment5976>

    What is this method's policy on watches?  Please note it in javadoc.  This method is not
at all strict and is multiple operations so is not safe to use on nodes where we rely on watches
/ monitoring of state transitions, so let's make some kind of note.


- Jonathan


On 2010-11-08 11:47:12, stack wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1187/
> -----------------------------------------------------------
> 
> (Updated 2010-11-08 11:47:12)
> 
> 
> Review request for hbase and Jonathan Gray.
> 
> 
> Summary
> -------
> 
> Renamed ZKTableDisable as ZKTable, making it a generic zk util for managing 'tables'.
> Added enabing/disabling states to table the current set of enabled/disabled only.
> 
> M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
>   (createSetData): Added.
> M src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
>   Removed offlining region utility methods no longer used.
>   (We do it now over in MetaEditor)
> M src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
>   Javadoc.
> M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
>   Add a base abstract class to do 'bulk assignments'.  Redo
>   assignAllUserRegions to use subclass of new bulk assigner class.
>   Added isTableEnabled, disablingTable, enablingTable.
> M src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java
>   Redid to use new bulk assigner class.
> M src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java
> A Added TestZKTable
> 
> 
> This addresses bug hbase-3112.
>     http://issues.apache.org/jira/browse/hbase-3112
> 
> 
> Diffs
> -----
> 
>   trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032652

>   trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java
1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java
1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032652 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032652 
>   trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032652

>   trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION

> 
> Diff: http://review.cloudera.org/r/1187/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> stack
> 
>


Mime
View raw message