> On 2010-06-11 17:35:09, Todd Lipcon wrote: > > 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.
Thanks for review Todd. In ZK refactor, we will do in-depth look at where we should be throwing RTE vs logging/swallowing vs throwing. When we do reimplementation of the open/close and all that, will look at the one you pointed out. Applied the following changes per your comments: - Switched to foreach in event handler - Switch to NONE.value - Added TODOs for using guava and reviewing IOE vs LOG - Added license headers to ZKUnassignedWatcher, TestZKBasedReopenRegion, TestZKBasedCloseRegion - Added logging to noop closing method Will put final patch up onto JIRA and commit. Thanks! > On 2010-06-11 17:35:09, Todd Lipcon wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java, line > > 1000 > > <http://review.hbase.org/r/171/diff/1/?file=1267#file1267line1000> > > > > are these IOEs here and above really recoverable in any way? How could > > they happen? They wouldn't happen unless the Writable implementations were broken. Not sure what we should be doing here besides logging? Better to log and ignore or throw RTE to bring attention to something being very broken? Adding TODO to review what we should do here - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/171/#review202 ----------------------------------------------------------- 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 > >
