You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by st...@duboce.net on 2010/11/08 20:38:31 UTC
Review Request: hbase-3112 Enable and disable of table needs a bit of loving
in new master
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/
-----------------------------------------------------------
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/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
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
> On 2010-11-08 11:50:55, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1143
> > <http://review.cloudera.org/r/1187/diff/1/?file=16872#file16872line1143>
> >
> > What does this mean? done vs. not done? I think we should be more descriptive in the logging (if done, then we've completed assignment of regions on cluster startup). But if not done, on startup, what does this mean? There's comment later that RIT timeouts should fix it up, so should be in log message here? Or on startup case of bulk assign, should we fail startup here if this doesn't pass?
Removed it. It confuses (see above for exhibit A).
If problem doing bulk assign, we'll crash out master. Otherwise, timeout of RIT should fix bulk assign stragglers.
> On 2010-11-08 11:50:55, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1149
> > <http://review.cloudera.org/r/1187/diff/1/?file=16872#file16872line1149>
> >
> > Can we move all this stuff into a separate class? AssignmentManager is getting huge. Maybe BulkAssign could be class that contains all these other class definitions?
> >
> > Also gives good opportunity in class comment to describe in general how this stuff works.
BulkAssigner class comment says -- perhaps a little curtly -- what it does? I'll move it out. The implementations though I'll leave beside where they are used -- in class.
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1840
-----------------------------------------------------------
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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1840
-----------------------------------------------------------
Going to continue review on v2 patch
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5962>
whitespace (and we don't have IOException in javadoc but that's not your fault)
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5964>
What does this mean? done vs. not done? I think we should be more descriptive in the logging (if done, then we've completed assignment of regions on cluster startup). But if not done, on startup, what does this mean? There's comment later that RIT timeouts should fix it up, so should be in log message here? Or on startup case of bulk assign, should we fail startup here if this doesn't pass?
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1187/#comment5961>
Can we move all this stuff into a separate class? AssignmentManager is getting huge. Maybe BulkAssign could be class that contains all these other class definitions?
Also gives good opportunity in class comment to describe in general how this stuff works.
- 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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
> On 2010-11-08 17:04:25, Jonathan Gray wrote:
> > 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.
I want the state-checking methods to flag bad state transitions.
On hitting zk on each assign, pardon me, I didn't grok what you were on about when you talked this up earlier. I get it now. Will fix.
Tests are failing because I changed sematics; disable/enable are now async where before they were sync. Will do your suggestion of creating a sync version if only to make tests pass (should only be for use of tests).
> On 2010-11-08 17:04:25, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 447
> > <http://review.cloudera.org/r/1187/diff/3/?file=16953#file16953line447>
> >
> > 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?
ok. removed mention of states.
> On 2010-11-08 17:04:25, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 659
> > <http://review.cloudera.org/r/1187/diff/3/?file=16955#file16955line659>
> >
> > 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.
Will fix in next patch.
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1853
-----------------------------------------------------------
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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
> On 2010-11-09 11:05:46, Jean-Daniel Cryans wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 444
> > <http://review.cloudera.org/r/1187/diff/5/?file=17034#file17034line444>
> >
> > I gave you my comments in person. Short version, I think that those methods' method shouldn't change and that we should have methods clearly marked as "async", and then do a job of educating people towards using them.
>
> Jean-Daniel Cryans wrote:
> I meant method's behavior
Yeah, I agree with you after chatting. Will fix (And you spotted prob. w/ way async was running anyways).
> On 2010-11-09 11:05:46, Jean-Daniel Cryans wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java, line 135
> > <http://review.cloudera.org/r/1187/diff/5/?file=17043#file17043line135>
> >
> > Looks an awful lot like BulkDisabler
I disagree. The overrides each differ substantially (They look similar if you don't look close -- smile).
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1866
-----------------------------------------------------------
On 2010-11-09 09:50:33, stack wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1187/
> -----------------------------------------------------------
>
> (Updated 2010-11-09 09:50:33)
>
>
> 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 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/BulkAssigner.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032841
> trunk/src/main/ruby/hbase/admin.rb 1032841
> trunk/src/main/ruby/shell.rb 1032841
> trunk/src/main/ruby/shell/commands/disable.rb 1032841
> trunk/src/main/ruby/shell/commands/enable.rb 1032841
> trunk/src/main/ruby/shell/commands/is_disabled.rb PRE-CREATION
> trunk/src/main/ruby/shell/commands/is_enabled.rb PRE-CREATION
> trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1187/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> stack
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by Jean-Daniel Cryans <jd...@apache.org>.
> On 2010-11-09 11:05:46, Jean-Daniel Cryans wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 444
> > <http://review.cloudera.org/r/1187/diff/5/?file=17034#file17034line444>
> >
> > I gave you my comments in person. Short version, I think that those methods' method shouldn't change and that we should have methods clearly marked as "async", and then do a job of educating people towards using them.
I meant method's behavior
- Jean-Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1866
-----------------------------------------------------------
On 2010-11-09 09:50:33, stack wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1187/
> -----------------------------------------------------------
>
> (Updated 2010-11-09 09:50:33)
>
>
> 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 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/BulkAssigner.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032841
> trunk/src/main/ruby/hbase/admin.rb 1032841
> trunk/src/main/ruby/shell.rb 1032841
> trunk/src/main/ruby/shell/commands/disable.rb 1032841
> trunk/src/main/ruby/shell/commands/enable.rb 1032841
> trunk/src/main/ruby/shell/commands/is_disabled.rb PRE-CREATION
> trunk/src/main/ruby/shell/commands/is_enabled.rb PRE-CREATION
> trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1187/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> stack
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by Jean-Daniel Cryans <jd...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1866
-----------------------------------------------------------
trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<http://review.cloudera.org/r/1187/#comment6054>
I gave you my comments in person. Short version, I think that those methods' method shouldn't change and that we should have methods clearly marked as "async", and then do a job of educating people towards using them.
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java
<http://review.cloudera.org/r/1187/#comment6052>
Looks an awful lot like BulkDisabler
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java
<http://review.cloudera.org/r/1187/#comment6053>
A few methods need javadoc
- Jean-Daniel
On 2010-11-09 09:50:33, stack wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1187/
> -----------------------------------------------------------
>
> (Updated 2010-11-09 09:50:33)
>
>
> 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 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/BulkAssigner.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032841
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032841
> trunk/src/main/ruby/hbase/admin.rb 1032841
> trunk/src/main/ruby/shell.rb 1032841
> trunk/src/main/ruby/shell/commands/disable.rb 1032841
> trunk/src/main/ruby/shell/commands/enable.rb 1032841
> trunk/src/main/ruby/shell/commands/is_disabled.rb PRE-CREATION
> trunk/src/main/ruby/shell/commands/is_enabled.rb PRE-CREATION
> trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032841
> trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1187/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> stack
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/
-----------------------------------------------------------
(Updated 2010-11-09 09:50:33.321899)
Review request for hbase and Jonathan Gray.
Changes
-------
Ok. All table state is now managed by ZKTable, moved out of AM. It also keeps cache of states to save on zk ensemble roundtrips. I think I've addressed all of your review comments now Jon.
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 (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/BulkAssigner.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032841
trunk/src/main/ruby/hbase/admin.rb 1032841
trunk/src/main/ruby/shell.rb 1032841
trunk/src/main/ruby/shell/commands/disable.rb 1032841
trunk/src/main/ruby/shell/commands/enable.rb 1032841
trunk/src/main/ruby/shell/commands/is_disabled.rb PRE-CREATION
trunk/src/main/ruby/shell/commands/is_enabled.rb PRE-CREATION
trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1032841
trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032841
trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION
Diff: http://review.cloudera.org/r/1187/diff
Testing
-------
Thanks,
stack
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/
-----------------------------------------------------------
(Updated 2010-11-08 22:35:19.111130)
Review request for hbase and Jonathan Gray.
Changes
-------
Addressed Jon comments. Still to do is finish up synchronizations on cache passed into ZKTable, cache of table states to save on trips to zk ensemble. Also, need to run tests an put in place calls to synchronized enable/disable table methods that this patch adds so tests start passing again. Will do in morning.
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 (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/BulkAssigner.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java 1032841
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1032841
trunk/src/main/ruby/hbase/admin.rb 1032841
trunk/src/main/ruby/shell.rb 1032841
trunk/src/main/ruby/shell/commands/disable.rb 1032841
trunk/src/main/ruby/shell/commands/enable.rb 1032841
trunk/src/main/ruby/shell/commands/is_disabled.rb PRE-CREATION
trunk/src/main/ruby/shell/commands/is_enabled.rb PRE-CREATION
trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java 1032841
trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 1032841
trunk/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZKTable.java PRE-CREATION
Diff: http://review.cloudera.org/r/1187/diff
Testing
-------
Thanks,
stack
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/
-----------------------------------------------------------
(Updated 2010-11-08 16:09:44.239145)
Review request for hbase and Jonathan Gray.
Changes
-------
Added shell is_enabled, is_disabled. Did some fixup because of issues revealed testing. Implemented jgray suggestions.
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 (updated)
-----
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
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > 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.
Added to shell is_enabled and is_disabled. Added javadoc to enableTable and disableTable explaining how these methods return immediately now but that process can take a while to complete.
On methods that just touch zk, they are of a class of which one member -- isTableDisabled -- does checks of AM data structures... so I can't really move them out.
Change ZKTable to use enums.
If disabling and master failover, when new master comes online, will be broke. But enable/disable are idempotent and a rerun of enable/disable will start up the process again and it should finish off properly. Thats the theory anyways.
Regards unit tests, enable/disable is used all over unit test code base. If you are looking for a test that simulates big table enable/disable, that's hard to do in a unit test.
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1302
> > <http://review.cloudera.org/r/1187/diff/2/?file=16884#file16884line1302>
> >
> > I thought we had this method somewhere already?
there is a similarly named one for zk that you added -- is that what you are referring to?
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java, line 1573
> > <http://review.cloudera.org/r/1187/diff/2/?file=16884#file16884line1573>
> >
> > 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?)
See note above where argument is that these are of a set and that since one of the set members does AM machinations, then all belong in AM. For now I think its fine.
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java, line 97
> > <http://review.cloudera.org/r/1187/diff/2/?file=16888#file16888line97>
> >
> > 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?
Added logging to enable/disable.
Regards 'locks', these are my own local copies of these Lists. Also notion that regions in table count can change is sort of allowed.. Each time through loop we'll recheck .META. for enabling and we'll re-look at the list of table regions in AssignmentManager....
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTable.java, line 41
> > <http://review.cloudera.org/r/1187/diff/2/?file=16890#file16890line41>
> >
> > enum?
done
> On 2010-11-08 12:13:11, Jonathan Gray wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java, line 716
> > <http://review.cloudera.org/r/1187/diff/2/?file=16892#file16892line716>
> >
> > 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.
K. This method is for non-watched znode. Will add this to javadoc.
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/#review1841
-----------------------------------------------------------
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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: hbase-3112 Enable and disable of table needs a bit of
loving in new master
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1187/
-----------------------------------------------------------
(Updated 2010-11-08 11:47:12.895726)
Review request for hbase and Jonathan Gray.
Changes
-------
Patch was missing the new files.
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 (updated)
-----
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