You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Jonathan Gray <jg...@apache.org> on 2010/07/27 03:57:08 UTC
Review Request: HBASE-2695/HBASE-2696 The rest of it. Master fully on new ZK
stuff.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/387/
-----------------------------------------------------------
Review request for hbase, stack and Karthik Ranganathan.
Summary
-------
This is the rest of the master cleanup and zookeeper cleanup. Everything is moved over to the new ZooKeeperWatcher, ZooKeeperListeners, ZKUtil/ZKAssign, etc...
There is a second page to the diff linked at the bottom with lots of good stuff, don't miss it!
Now on to the good stuff!
This addresses bugs HBASE-2695 and HBASE-2696.
http://issues.apache.org/jira/browse/HBASE-2695
http://issues.apache.org/jira/browse/HBASE-2696
Diffs
-----
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/Abortable.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/MiniZooKeeperCluster.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RegionServerTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 964617
branches/0.90_master_rewrite/src/main/resources/hbase-webapps/master/master.jsp 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/TestMultiParallelPut.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/OOMEHMaster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/regionserver/TestMasterAddressManager.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java PRE-CREATION
Diff: http://review.hbase.org/r/387/diff
Testing
-------
Most unit tests passing. Still addressing remaining failures but most seem to be related to the fact that I was running multiple tests and ZK clusters were stomping on each other.
Thanks,
Jonathan
Re: Review Request: HBASE-2695/HBASE-2696 The rest of it. Master fully on
new ZK stuff.
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/387/
-----------------------------------------------------------
(Updated 2010-07-27 09:31:00.269382)
Review request for hbase, stack and Karthik Ranganathan.
Changes
-------
Just attaching to HBASE-2695 to see if it makes it to the lists now
Summary
-------
This is the rest of the master cleanup and zookeeper cleanup. Everything is moved over to the new ZooKeeperWatcher, ZooKeeperListeners, ZKUtil/ZKAssign, etc...
There is a second page to the diff linked at the bottom with lots of good stuff, don't miss it!
Now on to the good stuff!
This addresses bug HBASE-2695.
http://issues.apache.org/jira/browse/HBASE-2695
Diffs
-----
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/Abortable.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/MiniZooKeeperCluster.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RegionServerTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 964617
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java PRE-CREATION
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 964617
branches/0.90_master_rewrite/src/main/resources/hbase-webapps/master/master.jsp 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/TestMultiParallelPut.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/OOMEHMaster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/regionserver/TestMasterAddressManager.java 964617
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java PRE-CREATION
Diff: http://review.hbase.org/r/387/diff
Testing
-------
Most unit tests passing. Still addressing remaining failures but most seem to be related to the fact that I was running multiple tests and ZK clusters were stomping on each other.
Thanks,
Jonathan
Re: Review Request: HBASE-2695/HBASE-2696 The rest of it. Master fully on
new ZK stuff.
Posted by Todd Lipcon <to...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/387/#review518
-----------------------------------------------------------
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
<http://review.cloudera.org/r/387/#comment2070>
could you make this just be a field of the enum, and set it on in the enum constructor?
RS2ZK_REGION_CLOSING(1, "CLOSING")
the switch (this) just seems a little ugly. no big deal though.
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2071>
I guess it's out of scope, but I'd love it if these were moved to avro data, rather than Writables - it will really help with debuggability since we could tell it to use JSON serialization, and then we can easily inspect ZK using external tools without having to write them ourselves.
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2073>
add assert?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2074>
assert?
also consider maybe making these into factory methods to clarify their usage?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2075>
don't we usually capitalize this as Timestamp? for some reason this looks weird.
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2077>
else serverName = null
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2078>
else hmsg = null
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/387/#comment2080>
maybe add a boolean started=true, and assert to make sure we don't start twice, and checks elsewhere to make sure it gets started?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/387/#comment2081>
get result and assert it's empty, perhaps?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/387/#comment2082>
default: log it?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/387/#comment2083>
when would this happen?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/387/#comment2084>
again, when?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java
<http://review.cloudera.org/r/387/#comment2085>
UP_DATA
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2086>
assert path.startsWith(zkw.assignmentZNode)
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2087>
synchronizing on someone else's member seems error-prone here
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2090>
is it possible to attach a string to this exception?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2092>
should we throw exceptions instead of returning false for stuff like this? I don't know how exceptional this case is.
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2093>
again on returning false here - maybe an exception like UnexpectedZNodeStateException() so we can attach these nice warnings to the exception trace?
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2094>
this duality of argument is a little strange.
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
<http://review.cloudera.org/r/387/#comment2095>
do these synchronized blocks present possible deadlock issues?
I'm wondering about something like this:
Thread A:
synchronized (nodes) {
do some ZK op that starts a watch;
do some synchronous ZK op;
}
ZK thread:
get event that triggers watch
call watch handler
watch handler synchronizes on nodes and blocks
... blocked, so doesn't process further, so the above synchronous ZK op never returns
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
<http://review.cloudera.org/r/387/#comment2096>
typo: there
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java
<http://review.cloudera.org/r/387/#comment2097>
I think wait() on a public object is bad form... can you use an internal Object to wait/notify on, to be clear what the condition is you're waiting for?
also maybe best to wait(1000); "just in case"
branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
<http://review.cloudera.org/r/387/#comment2098>
hm?
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java
<http://review.cloudera.org/r/387/#comment2099>
TODO to re-enable?
branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java
<http://review.cloudera.org/r/387/#comment2100>
perhaps add LOGs to these stubs for handy debugging of test failures?
- Todd
On 2010-07-26 18:57:08, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/387/
> -----------------------------------------------------------
>
> (Updated 2010-07-26 18:57:08)
>
>
> Review request for hbase, stack and Karthik Ranganathan.
>
>
> Summary
> -------
>
> This is the rest of the master cleanup and zookeeper cleanup. Everything is moved over to the new ZooKeeperWatcher, ZooKeeperListeners, ZKUtil/ZKAssign, etc...
>
> There is a second page to the diff linked at the bottom with lots of good stuff, don't miss it!
>
> Now on to the good stuff!
>
>
> This addresses bugs HBASE-2695 and HBASE-2696.
> http://issues.apache.org/jira/browse/HBASE-2695
> http://issues.apache.org/jira/browse/HBASE-2696
>
>
> Diffs
> -----
>
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/Abortable.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/MiniZooKeeperCluster.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RegionServerTracker.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 964617
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java PRE-CREATION
> branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 964617
> branches/0.90_master_rewrite/src/main/resources/hbase-webapps/master/master.jsp 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/TestMultiParallelPut.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/OOMEHMaster.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/regionserver/TestMasterAddressManager.java 964617
> branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/387/diff
>
>
> Testing
> -------
>
> Most unit tests passing. Still addressing remaining failures but most seem to be related to the fact that I was running multiple tests and ZK clusters were stomping on each other.
>
>
> Thanks,
>
> Jonathan
>
>