[
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" <[email protected]>
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.
