[
https://issues.apache.org/jira/browse/HBASE-2201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12922735#action_12922735
]
HBase Review Board commented on HBASE-2201:
-------------------------------------------
Message from: [email protected]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1045/#review1577
-----------------------------------------------------------
Ship it!
+1 looks good. Minor comments below.
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5298>
Is this what enables replication on the cluster, setting this config?
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5297>
You need a <p> here and at start of all paragraphs starts that follow else
all runs together
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5299>
This is the name of a group of replication commands? There are more than
one?
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5300>
zkWrapper is a bad name? Its actually a Watcher? Elsewhere you call it
zkHelper
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5301>
You don't want to proceed though replication not enabled?
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5302>
No exceptions come up out of here? Interrupted Exception or
KeeperException?
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5303>
Throw is org.apache.commons.lang.NotImplementedException
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5304>
Return old state?
/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
<http://review.cloudera.org/r/1045/#comment5305>
Why let it out?
/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/1045/#comment5306>
This change was done already, FYI
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
<http://review.cloudera.org/r/1045/#comment5307>
I don't understand this second sentence.
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
<http://review.cloudera.org/r/1045/#comment5308>
Why not make this immutable class and pass this in to the Constructor? It
changes?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
<http://review.cloudera.org/r/1045/#comment5309>
Why let this out?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
<http://review.cloudera.org/r/1045/#comment5310>
ditto
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
<http://review.cloudera.org/r/1045/#comment5311>
public data member?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
<http://review.cloudera.org/r/1045/#comment5312>
Is this a silent fail? Should it be noisier?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
<http://review.cloudera.org/r/1045/#comment5313>
This parse stuff seems to be sprinkled about the code. Should we put it
together in one place? For example, it'll be needed when we fix copytable to
TOF can take clientPort for zk?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.cloudera.org/r/1045/#comment5314>
This abort used abort the server? Now you just terminate replication?
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.cloudera.org/r/1045/#comment5315>
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?)
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.cloudera.org/r/1045/#comment5316>
if (!this.replication.get()) return;
Then you don't have to indent whole method.
/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.cloudera.org/r/1045/#comment5317>
You don't need this.. its in super class?
/trunk/src/main/ruby/shell/commands/add_peer.rb
<http://review.cloudera.org/r/1045/#comment5318>
Good
- stack
> 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.
