----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/171/#review202 -----------------------------------------------------------
Ship it! Skimmed it reasonably quickly, looks good. Major concern is how often we catch and just log exceptions. In most cases I think these are errors we can't recover from and it's best to throw RTE and shut down the whole server. Several places missing license headers. trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java <http://review.hbase.org/r/171/#comment918> use foreach form? trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java <http://review.hbase.org/r/171/#comment919> foreach form trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java <http://review.hbase.org/r/171/#comment920> == NONE.value, or if (this == NONE)? trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java <http://review.hbase.org/r/171/#comment921> You can just use this one: http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/util/concurrent/NamingThreadFactory.html trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java <http://review.hbase.org/r/171/#comment922> are these IOEs here and above really recoverable in any way? How could they happen? trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java <http://review.hbase.org/r/171/#comment923> license trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java <http://review.hbase.org/r/171/#comment924> maybe add a LOG.debug here at least for now trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <http://review.hbase.org/r/171/#comment925> again, are these LOG.errors really recoverable things? trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java <http://review.hbase.org/r/171/#comment926> recoverable? trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java <http://review.hbase.org/r/171/#comment927> can't wait til we refactor all this stuff trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java <http://review.hbase.org/r/171/#comment928> license - Todd On 2010-06-11 16:24:15, Karthik Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/171/ > ----------------------------------------------------------- > > (Updated 2010-06-11 16:24:15) > > > Review request for hbase, Todd Lipcon and stack. > > > Summary > ------- > > Moved all RS -> Master communication to go through ZK so that we can enable > master failover. > > > This addresses bug HBASE-2694. > http://issues.apache.org/jira/browse/HBASE-2694 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java > PRE-CREATION > > trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java > PRE-CREATION > > trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java > PRE-CREATION > > trunk/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java > PRE-CREATION > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 953804 > trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionClose.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java > 953804 > > trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionStatusChange.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java > 953804 > > trunk/src/main/java/org/apache/hadoop/hbase/master/ZKMasterAddressWatcher.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java > PRE-CREATION > > trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java > PRE-CREATION > > trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java > PRE-CREATION > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 953804 > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java > PRE-CREATION > trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/HQuorumPeer.java > 953804 > trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java > 953804 > trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 953804 > trunk/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java 953804 > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 953804 > > trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java > PRE-CREATION > > trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedReopenRegion.java > PRE-CREATION > > Diff: http://review.hbase.org/r/171/diff > > > Testing > ------- > > Unit tested. > > > Thanks, > > Karthik > >
