> On 2010-07-15 10:12:34, stack wrote: > > Looks good. Nothing to test in this new code (though most is just > > replacing and adding new exception).
Yup. It also fixes client to follow new masters and adds in a bunch of new methods to ZKUtil (unraveling ZKWrapper mess). Next patch is finishing Master side and should be able to completely remove ZKWrapper and _all_ instances of Watchers/process() methods besides in ZooKeeperWatcher itself. Thanks for review! > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java, > > line 27 > > <http://review.hbase.org/r/324/diff/2/?file=2813#file2813line27> > > > > 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' Makes sense and I see the confusion. I think what you say below, an Abortable interface, probably makes good sense as the common base class for all of these. Server anything is bad on client unless it does it _to_ the server, which is not the case here. Will do this on the next patch. > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java, > > line 46 > > <http://review.hbase.org/r/324/diff/2/?file=2813#file2813line46> > > > > Is this javadoc right? Mentions serverstatus Will cleanup once we finalize > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, > > line 325 > > <http://review.hbase.org/r/324/diff/2/?file=2817#file2817line325> > > > > Should we remove this exception? Or deprecate it? The > > MasterNotRunning one? No, this is still valid. Clients have two general exceptions now. HTable will throw ZooKeeperConnectionException. HBaseAdmin will throw ZK and MasterNotRunningException. MNRE is for the case when ZK is up but either has no master address in it or is unable to connect to the master address in zk. > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, > > line 1681 > > <http://review.hbase.org/r/324/diff/2/?file=2819#file2819line1681> > > > > I'm for a super Interface. Its odd have server-side in here. How > > about Abortable Interface? +1 > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, > > line 1697 > > <http://review.hbase.org/r/324/diff/2/?file=2819#file2819line1697> > > > > There is a Configurable interface up in hadoop (but also has setConf > > which you don't want I'd say)... just FYI. Once we add Abortable from above, this will get removed. > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java, > > line 20 > > <http://review.hbase.org/r/324/diff/2/?file=2825#file2825line20> > > > > 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? Yes, it actually manages it. You point it at ZK and it exposes a method getMasterAddress() which will give you the up-to-date address from ZK (it doesn't actually read from ZK when you hit the method, it's being kept up to date automagically) > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java, > > line 1 > > <http://review.hbase.org/r/324/diff/2/?file=2826#file2826line1> > > > > Call this RSZKUpdater to match your pattern naming classes that have to > > do w/ zk (ZKUtil, ZKConf). I'm hoping to rip this apart and put it into Handlers where it belongs. Right now it's kinda mixed, some stuff is ZK some stuff is ZooKeeper. ZooKeeper is just so friggin long ;) > On 2010-07-15 10:12:34, stack wrote: > > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java, > > line 54 > > <http://review.hbase.org/r/324/diff/2/?file=2830#file2830line54> > > > > Do you mean ensemble? I used previous naming convention (all the existing ZK methods are about getting the quorum string). You are probably right, it's the ensemble but would change a decent bit of existing code so not sure if we should change or not. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/324/#review404 ----------------------------------------------------------- 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 > >
