hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: HBASE-2696: ZK cleanup. Finish RS and fix client, move to ZKWatcher
Date Thu, 15 Jul 2010 17:12:33 GMT

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


Mime
View raw message