----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/324/#review404 -----------------------------------------------------------
Ship it! Looks good. Nothing to test in this new code (though most is just replacing and adding new exception). branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java <http://review.hbase.org/r/324/#comment1716> Client "is a" ServerController or do you mean that you can do ServerController actions via client? (I see later that whats going on here is that the client is proxying Server calls) Also, (don't kill me), but controller is bad name for this interface. Going by the current set of methods in this interface, they are methods an actual controller would make use of; e.g. a "Controller" would decide its time to abort the server. I still think it should be 'Server' or 'Process' branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java <http://review.hbase.org/r/324/#comment1717> Is this javadoc right? Mentions serverstatus branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java <http://review.hbase.org/r/324/#comment1718> Should we remove this exception? Or deprecate it? The MasterNotRunning one? branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java <http://review.hbase.org/r/324/#comment1719> Having this bit of code inside each method was really bad... thats good you fixed it branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java <http://review.hbase.org/r/324/#comment1720> I'm for a super Interface. Its odd have server-side in here. How about Abortable Interface? branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java <http://review.hbase.org/r/324/#comment1721> There is a Configurable interface up in hadoop (but also has setConf which you don't want I'd say)... just FYI. branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java <http://review.hbase.org/r/324/#comment1722> Sorry, don't remember what I said in last review but did we agree that this class actually 'manages' the master address? It has the watcher and updates itself if change? That kinda thing? branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java <http://review.hbase.org/r/324/#comment1723> Call this RSZKUpdater to match your pattern naming classes that have to do w/ zk (ZKUtil, ZKConf). branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java <http://review.hbase.org/r/324/#comment1726> Do you mean ensemble? branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java <http://review.hbase.org/r/324/#comment1727> Should probably be ensemble but probably a big change doing that? - stack On 2010-07-15 08:52:42, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/324/ > ----------------------------------------------------------- > > (Updated 2010-07-15 08:52:42) > > > Review request for hbase, stack, Jean-Daniel Cryans, and Karthik Ranganathan. > > > Summary > ------- > > Next set of changes for ZK cleanup (following master startup cleanup in > HBASE-2695). > > This completes move from ZooKeeperWrapper to ZooKeeperWatcher (and all the > new ZK bits) for the RS and Client. There is still work to do in Master, > that is next patch which hopefully comes today. > > * Rename {{ServerStatus}} to {{ServerController}} (still have MasterStatus, > that will be renamed in next patch) > ** Client uses this now, maybe should be ProcessController? > * Adds a new client exception {{ZooKeeperConnectionException}} which is > subclass of IOException. Thrown from HTable and HBaseAdmin if no ZK > connection > * Fixes client so that it can ride over master changes > ** Uses {{MasterAddressManager}} already being used on RS > ** {{HBaseAdmin}} uses accessor method to get master reference rather than a > class variable > ** Move all ZK interactions to go via ZKUtil/ZKWatcher not ZKWrapper > * Moves RS completely off of ZKWrapper and onto ZKUtil/ZKWatcher > * Adds lots of new methods to ZKUtil, every one with full javadoc > > > This addresses bug HBASE-2696. > http://issues.apache.org/jira/browse/HBASE-2696 > > > Diffs > ----- > > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java > PRE-CREATION > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerStatus.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ZooKeeperConnectionException.java > PRE-CREATION > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/avro/AvroServer.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnection.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HTable.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/ServerConnectionManager.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterStatus.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKConfig.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java > 964096 > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java > 964096 > branches/0.90_master_rewrite/src/main/resources/hbase-webapps/master/zk.jsp > 964096 > > branches/0.90_master_rewrite/src/main/resources/hbase-webapps/regionserver/regionserver.jsp > 964096 > > branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java > 964096 > > branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java > 964096 > > branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java > 964096 > > branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java > 964096 > > branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/regionserver/TestMasterAddressManager.java > 964096 > > Diff: http://review.hbase.org/r/324/diff > > > Testing > ------- > > Had to fix up some of the ZK unit tests to work on new ZKUtil/ZKWatcher, all > those passing. Existing unit tests on MasterAddressManager. > > Test suite seems to be passing, underway. > > > Thanks, > > Jonathan > >
