hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-6036) Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls)
Date Sat, 19 May 2012 00:10:32 GMT

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

jiraposter@reviews.apache.org commented on HBASE-6036:
------------------------------------------------------



bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > Patch looks good.  Minor nits below.  See what you think.

Thanks for the review.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 704
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line704>
bq.  >
bq.  >     FYI, convention is space after the comma -- its easier to read.

Fixed.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1657
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line1657>
bq.  >
bq.  >     You should write this as
bq.  >     
bq.  >     LOG.info("Checking master connection", e);
bq.  >     
bq.  >     Should it be warn?

Changed.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1208
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1208>
bq.  >
bq.  >     White space

Fixed.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 82
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line82>
bq.  >
bq.  >     Why we take it if unused?

protobuf generates the function signature like that.  When I implement HBASE-6039, I'm going
to just take the functions as they are generated by protobufs (see RegionServerStatusProtocol
for an example).


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1761
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1761>
bq.  >
bq.  >     No need of the controller?  Would we ever need it?  If not, don't pass it?

Covered above.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/protobuf/Master.proto, line 56
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line56>
bq.  >
bq.  >     Should this be MasterRunningRequest rather than IsMasterRunningRequest?
bq.  >     
bq.  >     Or, is it just that you have a pattern going here where the Messages match the
rpc in name?
bq.  >     
bq.  >     If the latter, thats good enough for me.

It's the latter.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1077
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1077>
bq.  >
bq.  >     Two spaces in hbase and hadoop for 'tab' .  This 'return' is 4 or 6 spaces over?

Fixed.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 236
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line236>
bq.  >
bq.  >     Nit: You should look at the javadoc generated by this markup.  You'll see that
it comes out nothing like how you have it here formatted.  For future.

Fixed, thanks for pointing that out.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 1433
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109491#file109491line1433>
bq.  >
bq.  >     So, its ok changing the public-facing API because 0.96 is going to be the singularity?

That's true, but no point in breaking clients of this class if we don't need to.


bq.  On 2012-05-18 22:43:10, Michael Stack wrote:
bq.  > src/main/protobuf/Master.proto, line 133
bq.  > <https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line133>
bq.  >
bq.  >     This method and message name is awkward.  To match your IsMasterRunning, this
should be IsBalancer?

Agree that it is awkward.  IsMasterRunning isn't a good match, because that is a question,
whereas this controls whether the load balancer should be on or off (that is, an action).
 This is the old balanceSwitch(boolean).

How about setBalancerRunning(boolean)? 


- Gregory


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5157/#review7982
-----------------------------------------------------------


On 2012-05-17 20:33:52, Gregory Chanan wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/5157/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-05-17 20:33:52)
bq.  
bq.  
bq.  Review request for hbase and Michael Stack.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Convert the cluster-level calls that do not touch the file-format related calls (see
HBASE-5453). These are:
bq.  IsMasterRunning
bq.  Shutdown
bq.  StopMaster
bq.  Balance
bq.  LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)
bq.  
bq.  
bq.  This addresses bug HBASE-6036.
bq.      https://issues.apache.org/jira/browse/HBASE-6036
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af 
bq.    src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 
bq.    src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20 
bq.    src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403

bq.    src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06 
bq.    src/main/protobuf/Master.proto PRE-CREATION 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
a24f937 
bq.  
bq.  Diff: https://reviews.apache.org/r/5157/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Ran unit tests, all passed.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Gregory
bq.  
bq.


                
> Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls)
> --------------------------------------------------------------------------------------
>
>                 Key: HBASE-6036
>                 URL: https://issues.apache.org/jira/browse/HBASE-6036
>             Project: HBase
>          Issue Type: Task
>          Components: ipc, master, migration
>            Reporter: Gregory Chanan
>            Assignee: Gregory Chanan
>             Fix For: 0.96.0
>
>         Attachments: HBASE-6036.patch
>
>
> This should be a subtask of HBASE-5445, but since that is a subtask, I can't also make
this a subtask (apparently).
> Convert the cluster-level calls that do not touch the file-format related calls (see
HBASE-5453).  These are:
> IsMasterRunning
> Shutdown
> StopMaster
> Balance
> LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message