[
https://issues.apache.org/jira/browse/HBASE-5444?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245013#comment-13245013
]
jirapos...@reviews.apache.org commented on HBASE-5444:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4463/#review6651
-----------------------------------------------------------
Looks great. On commit will everything be broke?
pom.xml
<https://reviews.apache.org/r/4463/#comment14348>
We don't need this anymore now we are checking in generated files.
src/main/java/org/apache/hadoop/hbase/HConstants.java
<https://reviews.apache.org/r/4463/#comment14349>
Should this top level class have references down into protobufs? Maybe the
empty server load should be in ServerLoad or at least under o.a.h.h.protobuf
src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
<https://reviews.apache.org/r/4463/#comment14350>
Radical!
Jimmy didn't remove his interface. Maybe he should have?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4463/#comment14351>
Should it return the Response builder or the Response (don't they have same
underlying 'Interface'? Return that?)
Oh, I think I understand... must be a Builder in case we add config later?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4463/#comment14352>
ok, yeah, here is the actual instance, not the builder
src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java
<https://reviews.apache.org/r/4463/#comment14354>
You think this package right place for this? How about in master package
or up above at o.a.h.h?
Is MasterRegionInterface a good name for this Interface even? The name was
made long long time ago. Didn't make sense then. Still doesn't. Should it be
called RegionServerInterface or RegionServer in the master package -- its the
Interface RegionServers invoke on the master... whats a good name for that?
RegionServerService or RegionServersInterface
src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
<https://reviews.apache.org/r/4463/#comment14353>
Something called ProtobufUtil.java was added since you posted your patch.
Maybe this stuff belongs in there?
src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
<https://reviews.apache.org/r/4463/#comment14355>
Why not have this as static in ServerLoad?
src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
<https://reviews.apache.org/r/4463/#comment14356>
ditto
src/main/proto/MasterRegionProtocol.proto
<https://reviews.apache.org/r/4463/#comment14357>
MasterRegionProtocol doesn't seem like good name for this file?
MasterRegionServer or RegionServerToMasterProtocol or Interface?
Jimmy left of the 'Protocol' in his patch.
src/main/proto/MasterRegionProtocol.proto
<https://reviews.apache.org/r/4463/#comment14358>
Seems like a bad name. No Region on Master?
src/main/proto/MasterRegionProtocol.proto
<https://reviews.apache.org/r/4463/#comment14359>
What is this again? Should this be ServerName from hbase.proto?
src/main/proto/MasterRegionProtocol.proto
<https://reviews.apache.org/r/4463/#comment14360>
ditto
src/main/proto/MasterRegionProtocol.proto
<https://reviews.apache.org/r/4463/#comment14361>
RegionServerService?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4463/#comment14362>
A file of this name has gone in already. Need to rejigger your patch?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4463/#comment14363>
Use Jimmy's regionspec?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4463/#comment14364>
I think jimmy made one of these already. Coordinate. I like the name of
yours...
- Michael
On 2012-03-23 19:55:28, Gregory Chanan wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4463/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-23 19:55:28)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Adds PB-based calls replacing HMasterRegionInterface.
bq.
bq. There are some temporary hacks, e.g. converting PB-based ServerLoad to
existing HServerLoad so I didn't need to convert ClusterStatus (which brings in
a lot of other changes). That will be cleaned up in HBASE-5445.
bq.
bq.
bq. This addresses bug HBASE-5444.
bq. https://issues.apache.org/jira/browse/HBASE-5444
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. pom.xml 10b13ef
bq.
src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
69434f7
bq.
src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
ae76204
bq. src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10
bq. src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347
bq. src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
cbfa489
bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68
bq. src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
fd97830
bq. src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0
bq. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
d47ef10
bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f
bq. src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
bq. src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
bq. src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java
be63838
bq. src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7
bq.
src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
e0af8fb
bq. src/main/proto/MasterRegionProtocol.proto PRE-CREATION
bq. src/main/proto/hbase.proto PRE-CREATION
bq. src/main/resources/hbase-webapps/master/table.jsp 811df46
bq. src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188
bq. src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java
368a0e5
bq. src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
f2f8ee3
bq. src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
841649a
bq. src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java
e99d251
bq.
bq. Diff: https://reviews.apache.org/r/4463/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Ran jenkins job, all unit tests passed.
bq.
bq.
bq. Thanks,
bq.
bq. Gregory
bq.
bq.
> Add PB-based calls to HMasterRegionInterface
> --------------------------------------------
>
> Key: HBASE-5444
> URL: https://issues.apache.org/jira/browse/HBASE-5444
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Gregory Chanan
>
--
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
