----------------------------------------------------------- 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 > >
