[
https://issues.apache.org/jira/browse/HBASE-6036?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13279355#comment-13279355
]
jirapos...@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
