hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "HBase Review Board (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HBASE-2696) ZooKeeper cleanup and refactor
Date Thu, 15 Jul 2010 20:27:57 GMT

    [ https://issues.apache.org/jira/browse/HBASE-2696?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888910#action_12888910
] 

HBase Review Board commented on HBASE-2696:
-------------------------------------------

Message from: "Jonathan Gray" <jgray@apache.org>


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > 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!


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java,
line 27
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2813#file2813line27>
bq.  >
bq.  >     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)
bq.  >     
bq.  >     
bq.  >     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.
bq.  >     
bq.  >     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.


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java,
line 46
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2813#file2813line46>
bq.  >
bq.  >     Is this javadoc right?  Mentions serverstatus

Will cleanup once we finalize


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java,
line 325
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2817#file2817line325>
bq.  >
bq.  >     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.


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java,
line 1681
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2819#file2819line1681>
bq.  >
bq.  >     I'm for a super Interface.  Its odd have server-side in here.  How about Abortable
Interface?

+1


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java,
line 1697
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2819#file2819line1697>
bq.  >
bq.  >     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.


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java,
line 20
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2825#file2825line20>
bq.  >
bq.  >     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)


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java,
line 1
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2826#file2826line1>
bq.  >
bq.  >     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 ;)


bq.  On 2010-07-15 10:12:34, stack wrote:
bq.  > branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java,
line 54
bq.  > <http://review.hbase.org/r/324/diff/2/?file=2830#file2830line54>
bq.  >
bq.  >     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
-----------------------------------------------------------





> ZooKeeper cleanup and refactor
> ------------------------------
>
>                 Key: HBASE-2696
>                 URL: https://issues.apache.org/jira/browse/HBASE-2696
>             Project: HBase
>          Issue Type: Sub-task
>          Components: master, regionserver, zookeeper
>            Reporter: Jonathan Gray
>            Assignee: Jonathan Gray
>            Priority: Critical
>             Fix For: 0.90.0
>
>         Attachments: HBASE-2696-part1-NewClasses_NotIntegrated.patch, HBASE-2696-part1-v2-NewClasses_NotIntegrated.patch,
HBASE-2696-part1-v3-NewClasses_RS.patch, HBASE-2696-part1-v4-NewClasses_RS_Tested.patch, HBASE-2696-part1-v5-NewClasses_RS_Tested.patch
>
>
> Currently almost everything we do with ZooKeeper is stuffed into a single class {{ZookeeperWrapper}}.
> This issue will deal with cleaning up our usage of ZK, adding some new abstractions to
help with the master changes, splitting up watchers from utility methods, and nailing down
the contracts of our ZK methods with respect to setting watchers, throwing exceptions, etc...

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message