[
https://issues.apache.org/jira/browse/HBASE-5443?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13218601#comment-13218601
]
[email protected] commented on HBASE-5443:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review5407
-----------------------------------------------------------
High-level, do we need to split the Interface more -- into admin vs user
protos? Would love to get rid of the plethora of methods but probably not a
task to be done as part of this issue?
pom.xml
<https://reviews.apache.org/r/4054/#comment11742>
There are prereqs for this to work? I suppose compile-proto.sh checks and
its failing it seems fails the build.. thats good.
pom.xml
<https://reviews.apache.org/r/4054/#comment11741>
Do we do this elsewhere in the pom? If so, should set a boolean once?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11743>
So, all protobuf files go here? We need to ensure this the case w/ all
patches (I think the rpc patch was putting them elsewhere.....)
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11744>
Elsewhere in hbase the convention is 'what' and then the generated classes
are in a 'generated' subpackage as in there is a thrift subpackage and under
there are thrift utils and then a 'generated' subpackage. Ditto avro. This is
different in that we have a generated top level subpackage w/ proto as a
subpackage. Should we flip it?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11745>
Should the suffix be Proto -- our convention for Protobuf classes (?) -- or
Protos? Why the plural? Perhaps this a container for a bunch of Proto?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11755>
This class seems to have more than HR stuff in it. Should we make more
protos? A client proto?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11751>
Mutation (Probably already used)
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11752>
What is this? For filters?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11753>
Mutate is not deprecated? We don't have deprecated stuff in this proto
file? This is doing what from current API?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11756>
Are these client datastructures? Or they go w/ the RS proto and are used
by clients?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11754>
Where does this come from in current API?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11758>
Doesn't HRI have a tablename in it? So maybe this should have a HRI?
What is this LogKeyProto modeling? Should be WALKeyProto?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11759>
WALEntryProto?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11760>
Should there be more in here?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11761>
Is this a class or method name? If method name is convention to capitalize?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11762>
So, even if method returns a HRegionInfo, we return a
GetRegionInfoResponseProto in case the response changes?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11763>
regionName and encodedName and HRegionInfo class.... should we standardize?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11764>
Excellent. Later we need to return first set of results in here rather
than have to have client go back to do a next.
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11765>
Maybe NextOnScannerRequestProto so relates better to our current API
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11766>
We should make this optional some day.. that you have to call a close.
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11767>
Man. Anyone use this thing?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11768>
We should have one or the other. Can have one call through to the other
(thats implementation?)
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11769>
Oh, I see how method naming and classes goes
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11770>
I wonder if these could take a more primitive type... a KV type.
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11771>
nextOnScanner?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11772>
These are admin interface functions. Should we split the interface?
src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11773>
Yeah, we need to doc. this later. Ain't the doc here going to be our
Interface doc? Our only Interface doc?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11750>
Doesn't an HRI have a Map?
There does not seem to be enough in here.
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11748>
My guess is that there is no uint32?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11746>
Whats a 'name'? Is it a region name?
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11747>
Should this be uint32? (Maybe not possible in proto?)
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11749>
I think a bit of doc in here would not go amiss. Else its hard to figure
all is going on in here; what goes where.
Should ServerName be in this proto file?
- Michael
On 2012-02-27 18:54:31, Jimmy Xiang wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4054/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-27 18:54:31)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This is the first draft of the ProtoBuff HRegionProtocol. The
corresponding java vs pb method mapping is attached to the jira:
https://issues.apache.org/jira/browse/HBASE-5443
bq.
bq. Please review. I'd like to move ahead after we get to some agreement.
bq.
bq.
bq. This addresses bug HBASE-5443.
bq. https://issues.apache.org/jira/browse/HBASE-5443
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. pom.xml 066c027
bq. src/main/proto/HRegionProtocol.proto PRE-CREATION
bq. src/main/proto/hbase.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4054/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Jimmy
bq.
bq.
> Add PB-based calls to HRegionInterface
> --------------------------------------
>
> Key: HBASE-5443
> URL: https://issues.apache.org/jira/browse/HBASE-5443
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: region_java-proto-mapping.pdf
>
>
--
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
