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-2201) JRuby shell for replication
Date Tue, 19 Oct 2010 21:41:28 GMT

    [ 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: stack@duboce.net

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


Mime
View raw message