[
https://issues.apache.org/jira/browse/HBASE-5444?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13259959#comment-13259959
]
[email protected] commented on HBASE-5444:
------------------------------------------------------
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > Looks great. On commit will everything be broke?
Should be able to commit next version; this version passed all the tests
previously (things have changed since then).
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > pom.xml, line 797
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95085#file95085line797>
bq. >
bq. > We don't need this anymore now we are checking in generated files.
Correct.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 33
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95089#file95089line33>
bq. >
bq. > 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
I'll put it under ProtobufUtil. Don't want to put it under ServerLoad because
that is generated (I know we are checking in the generated files, but it would
be a pain to maintain).
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 946
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95096#file95096line946>
bq. >
bq. > Should it return the Response builder or the Response (don't they
have same underlying 'Interface'? Return that?)
bq. >
bq. > Oh, I think I understand... must be a Builder in case we add config
later?
Yes to the second question.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 30
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line30>
bq. >
bq. > Something called ProtobufUtil.java was added since you posted your
patch. Maybe this stuff belongs in there?
Correct.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 56
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line56>
bq. >
bq. > Why not have this as static in ServerLoad?
Because ServerLoad is generated; would be a pain if we need to change it in the
future.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 79
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line79>
bq. >
bq. > ditto
Same as above.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 22
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line22>
bq. >
bq. > A file of this name has gone in already. Need to rejigger your
patch?
Yes.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 30
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line30>
bq. >
bq. > Use Jimmy's regionspec?
Good call.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/MasterRegionProtocol.proto, line 50
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line50>
bq. >
bq. > What is this again? Should this be ServerName from hbase.proto?
Will do.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/MasterRegionProtocol.proto, line 61
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line61>
bq. >
bq. > ditto
Will do.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. >
src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line
39
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95101#file95101line39>
bq. >
bq. > You think this package right place for this? How about in master
package or up above at o.a.h.h?
bq. >
bq. > 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
Master package sounds like a good place for it.
Agreed that the current name isn't good. When Jimmy split the interfaces to
the region, he broke them along functionality lines, i.e. Client vs Admin.
Maybe it makes sense to add something into the name that describes what the
interface does functionally, rather than just the endpoints. How about
RegionServerStatus{Service,Interface,Protocol}? (I'll just copy the endings
that Jimmy used). Every call in the interface basically describes the status,
e.g. startup, report, reportOnError.
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/MasterRegionProtocol.proto, line 1
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line1>
bq. >
bq. > MasterRegionProtocol doesn't seem like good name for this file?
MasterRegionServer or RegionServerToMasterProtocol or Interface?
bq. >
bq. > Jimmy left of the 'Protocol' in his patch.
How about RegionServerStatus here too? (with proper ending)
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/MasterRegionProtocol.proto, line 22
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line22>
bq. >
bq. > Seems like a bad name. No Region on Master?
RegionServerStatus here too (with proper ending)?
bq. On 2012-04-03 06:07:58, Michael Stack wrote:
bq. > src/main/proto/MasterRegionProtocol.proto, line 70
bq. > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line70>
bq. >
bq. > RegionServerService?
RegionServerStatus here too (with proper ending)?
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4463/#review6651
-----------------------------------------------------------
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
