[
https://issues.apache.org/jira/browse/HBASE-2201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12922774#action_12922774
]
HBase Review Board commented on HBASE-2201:
-------------------------------------------
Message from: "Jean-Daniel Cryans" <[email protected]>
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 35
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line35>
bq. >
bq. > Is this what enables replication on the cluster, setting this config?
yes
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 55
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line55>
bq. >
bq. > This is the name of a group of replication commands? There are more
than one?
This is a group yes, more than 1 command.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 59
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line59>
bq. >
bq. > zkWrapper is a bad name? Its actually a Watcher? Elsewhere you
call it zkHelper
true that!
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 68
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line68>
bq. >
bq. > You don't want to proceed though replication not enabled?
Well I want to proceed only if it's enabled.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 90
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line90>
bq. >
bq. > No exceptions come up out of here? Interrupted Exception or
KeeperException?
Let's bubble up a few IOEs.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 138
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line138>
bq. >
bq. > Why let it out?
It was laziness for the unit tests. I could just set it package protected?
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java,
line 1248
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14901#file14901line1248>
bq. >
bq. > This change was done already, FYI
cool thx
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java,
line 33
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14902#file14902line33>
bq. >
bq. > I don't understand this second sentence.
I meant that RP is a container class.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java,
line 92
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14902#file14902line92>
bq. >
bq. > Why not make this immutable class and pass this in to the
Constructor? It changes?
Laziness: let's say you add a new region server to a slave cluster, this list
will be regenerated from what's in Zookeeper. Instead of trying to figure out
which one is new (or missing), I just replace the whole list.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java,
line 100
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14902#file14902line100>
bq. >
bq. > Why let this out?
Since RP is a container class, getters are its only purpose.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java,
line 116
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14902#file14902line116>
bq. >
bq. > ditto
ditto too :)
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java,
line 86
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14903#file14903line86>
bq. >
bq. > public data member?
Yeah it's not pretty but that's also how ZooKeeperWatcher is done:
// base znode for this cluster
public String baseZNode;
// znode containing location of server hosting root region
public String rootServerZNode;
// znode containing ephemeral nodes of the regionservers
public String rsZNode;
// znode of currently active master
public String masterAddressZNode;
// znode containing the current cluster state
public String clusterStateZNode;
// znode used for region transitioning and assignment
public String assignmentZNode;
// znode used for table disabling/enabling
public String tableZNode;
Should I just put all those znodes public in RZ too?
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java,
line 255
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14903#file14903line255>
bq. >
bq. > Is this a silent fail? Should it be noisier?
No it's meant like that.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java,
line 244
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14904#file14904line244>
bq. >
bq. > This abort used abort the server? Now you just terminate
replication?
Well abort was calling terminate IIRC, and didn't really add value, so I just
refactored them together.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java,
line 637
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14904#file14904line637>
bq. >
bq. > Take a string which says why the termination triggered? Then in
here log at error level rather than sprinkle it throughout code (You seem to
preface the call to terminate with a Log.error and then in here do a log.info?)
There's also good reasons to terminate, but I'll take the hint and add a reason.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java,
line 458
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14906#file14906line458>
bq. >
bq. > You don't need this.. its in super class?
bq. >
ooops forgot to cleanup.
bq. On 2010-10-19 14:40:13, stack wrote:
bq. >
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java,
line 130
bq. > <http://review.cloudera.org/r/1045/diff/1/?file=14900#file14900line130>
bq. >
bq. > Return old state?
Will do, and will add a new getReplicating method.
- Jean-Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1045/#review1577
-----------------------------------------------------------
> JRuby shell for replication
> ---------------------------
>
> Key: HBASE-2201
> URL: https://issues.apache.org/jira/browse/HBASE-2201
> Project: HBase
> Issue Type: Sub-task
> Components: replication
> Reporter: Jean-Daniel Cryans
> Assignee: Jean-Daniel Cryans
> Fix For: 0.90.0
>
> Attachments: HBASE-2201-v2.patch, HBASE-2201.patch
>
>
> We need a shell to easily issue administration commands for replication. It
> should be easy to merge with existing core shell.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
