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 Tue, 09 Nov 2010 01:04:25 GMT

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

Ship it!


I can live without the state-checking methods in ZKTable but it's fine, no big deal as is.

Only issue is that we're hitting ZK on each assign().

Otherwise, if tests are passing and it's working for you up on cluster, I'm +1 on this.

If you add async/sync support, let's be sure to nail the javadoc.


trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<http://review.cloudera.org/r/1187/#comment6017>

    why describe 'enabling' state in our client API?  do we let out 'enabling' at all anywhere?
    
    and do we need to make mention that if it seems to never finish, this method should be
retried?



trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<http://review.cloudera.org/r/1187/#comment6019>

    same.  i think the javadoc is descriptive enough with this explanation of states.  just
not sure if we need any javadoc about retrying?



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

    isTableDisabling is actually a ZK read rather than checking in-memory state.  Should be
move enabling/disabling to in-memory state as well?  I'm not sure it's a good idea to have
to read from ZK on every assign() call, especially for something as rare as disabling.


- Jonathan


On 2010-11-08 16:09:44, stack wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1187/
> -----------------------------------------------------------
> 
> (Updated 2010-11-08 16:09:44)
> 
> 
> 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/HBaseAdmin.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/BulkAssigner.java PRE-CREATION 
>   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/main/ruby/hbase/admin.rb 1032778 
>   trunk/src/main/ruby/shell.rb 1032778 
>   trunk/src/main/ruby/shell/commands/disable.rb 1032778 
>   trunk/src/main/ruby/shell/commands/enable.rb 1032778 
>   trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.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