[
https://issues.apache.org/jira/browse/HBASE-5619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13238972#comment-13238972
]
[email protected] commented on HBASE-5619:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review6365
-----------------------------------------------------------
Excellent.
It looks like we can commit this w/o breaking whats currently there?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13813>
What did we figure on the package name? Shouldn't it agree w/ the dir that
holds the .proto files at src/main? Currently one is protobuf and the other is
proto.
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13835>
What are these two booleans broken out? Aren't they in they attributes of
HRI already? Why repeat them?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13836>
Good. Its kinda dumb the way our HRegionInterface is now where it has an
override, one that takes single family and another that takes an array. Thanks
for collapsing.
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13837>
Should we be repeating the API documentation that is up in the
HRegionInterface that this .proto replaces here? If its not here, where will
it be? Not all of the javadoc should make it over -- the stuff that says
nothing shouldn't but some is of worth. What you think?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13838>
Again, thanks for doing the work collapsing the overrides that are up in
HRegionInterface.
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13839>
Isn't the response currently a void? And isnt' flush async (IIRC). If
so, under what circumstance would we be able to fill out this response?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13842>
WALKey maps to HLogKey? Maybe add a comment to this effect?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13846>
Good. I like this method name better. Should be a comment which points
back to the old name? Or what you think?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13847>
Yeah, this proto is missing commentary. I mean, the return here should be
explained?
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13850>
This will for sure grow w/ time.
src/main/proto/RegionAdmin.proto
<https://reviews.apache.org/r/4054/#comment13855>
Nice. So you are splitting HRegionInterface into admin and client? Good.
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13862>
This is new? Being able to do this? How will it be used?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13863>
This is new feature on get? Or just special handling of an attribute?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13864>
We don't have this in the java Result. I don't understand why this is
making its way into the object.
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13865>
ditto
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13866>
what is processed?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13867>
Why we need to add a region to the Get even optionally?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13868>
Why is the Get polluted by multiGet stuff?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13869>
I thought we could set this into the Get above? Why have it here as
separate param?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13870>
Good stuff
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13872>
This is a new feature?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13881>
A type rather than have the mutation type specified as a subclass?
A mutate is a delete or put?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13882>
Again, does it make sense having this in here? I mean regions come and go
-- e.g. split -- so you could have reference to non-existent region. This
stuff of tying a mutation to a particular region should be done external to the
mutations themselves?
Is this trying to do multiaction? Maybe that should be a message apart
from mutate? The new message would have region name and the mutate?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13885>
What is this implementing from current Delete? The delete family?
This is returned to the client?
How do we do a column that has no qualifier? Thats possible in hbase.
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13890>
Yeah, why have regionspecified in the mutate if you are going to provide it
as a param too?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13898>
Why this when regions can change. Also, a Scan can traverse many regions
so what is the regionspecifier referring to? The first?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13899>
Is this supposed to be row specifier?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13900>
ditto
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13902>
Does this belong here?
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13905>
Does this belong here? You provide it as a param when making the request.
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13906>
ditto
src/main/proto/RegionClient.proto
<https://reviews.apache.org/r/4054/#comment13908>
Is this everything? I don't see closestrowbefore? We'll probably still
need this in 0.96.
src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment13909>
Good
- Michael
On 2012-03-26 20:14:22, 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-03-26 20:14:22)
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-5619.
bq. https://issues.apache.org/jira/browse/HBASE-5619
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. pom.xml 10b13ef
bq. src/main/proto/RegionAdmin.proto PRE-CREATION
bq. src/main/proto/RegionClient.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.
> Create PB protocols for HRegionInterface
> ----------------------------------------
>
> Key: HBASE-5619
> URL: https://issues.apache.org/jira/browse/HBASE-5619
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Jimmy Xiang
> Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5619.patch, hbase-5619_v3.patch
>
>
> Subtask of HBase-5443, separate HRegionInterface into admin protocol and
> client protocol, create the PB protocol buffer files
--
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
