[
https://issues.apache.org/jira/browse/HBASE-5619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239140#comment-13239140
]
[email protected] commented on HBASE-5619:
------------------------------------------------------
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > Excellent.
bq. >
bq. > It looks like we can commit this w/o breaking whats currently there?
That's right. It won't break anything. It is just proto files in this patch.
Thanks for reviewing.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 21
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line21>
bq. >
bq. > 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.
There is already an exiting folder called protobuf (rest). Let me change the
dir holds the .proto files under src/main to protobuf.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 35
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35>
bq. >
bq. > What are these two booleans broken out? Aren't they in they
attributes of HRI already? Why repeat them?
I used to put these transient parameters in the protobuff RegionInfo as well.
However Todd thought it's better to put them outside.
What'd do you think? To me, it is fine either way. However, if we are going
to replace HRI with the protobuff later on, it may be better to put them
together.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 70
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70>
bq. >
bq. > 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?
I though about this. I added documentation to RegionClient.proto. For methods
in RegionAdmin, the method names seem to be very clear. I will take a look
again and document where confusing/misunderstand could arise.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 87
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87>
bq. >
bq. > 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?
I was thinking to set it if the HRegion.flushCache call returned true. It is
just informational.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 112
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line112>
bq. >
bq. > WALKey maps to HLogKey? Maybe add a comment to this effect?
Sure. Will add a comment.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 141
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line141>
bq. >
bq. > Good. I like this method name better. Should be a comment which
points back to the old name? Or what you think?
Ok. Will do.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionAdmin.proto, line 151
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line151>
bq. >
bq. > Yeah, this proto is missing commentary. I mean, the return here
should be explained?
Ok, will add a comment too.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 52
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line52>
bq. >
bq. > This is new? Being able to do this? How will it be used?
Some call expects a region name, some call expects an encoded region name.
With a specifier, we can handle both.
Encoded region name works only if the region is on-line. If we can't find the
region based on the specifier, a region not found exception will be thrown.
So we can simplify the request a little bit since we don't have to ask for
region name, and encoded region name, and check only if one is specified.
I will add a comment for it.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 66
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66>
bq. >
bq. > This is new feature on get? Or just special handling of an
attribute?
This is for the exist() call. In this case, the caller doesn't care about the
result. They just want to know if the row is there. It is not special handling
of an attribute.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 71
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71>
bq. >
bq. > We don't have this in the java Result. I don't understand why this
is making its way into the object.
In the multi call, they mix many requests together. However, they don't
retrieve the response in order. They retrieve the response based on the region
name.
In this case, I'd like the server to return the region name. This is optional.
In most call, it is not used.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 74
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line74>
bq. >
bq. > ditto
It is used when the caller wants the existence only, not the result itself.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 77
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77>
bq. >
bq. > what is processed?
Sometimes, for some action, for example delete, they don't care about the
result. They just want to know if the action is processed or not. This is for
this kind use case.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 82
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82>
bq. >
bq. > Why we need to add a region to the Get even optionally?
That's to support multiple Get requests in one call. Each Get request can have
its own region. If all Gets are for one region, the caller can specify the
region in the request level.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 396
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line396>
bq. >
bq. > Is this everything? I don't see closestrowbefore? We'll probably
still need this in 0.96.
It is still there. It is collapsed into the Get call.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 156
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line156>
bq. >
bq. > A type rather than have the mutation type specified as a subclass?
bq. >
bq. > A mutate is a delete or put?
It could be a delete or a put.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 84
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>
bq. >
bq. > Why is the Get polluted by multiGet stuff?
The interface is kind of similar, but very flexible. Any problem with that?
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 91
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91>
bq. >
bq. > I thought we could set this into the Get above? Why have it here as
separate param?
This is used as a default region. If you have multiple Gets on the same
region, you don't have to specify it every where.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 147
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line147>
bq. >
bq. > This is a new feature?
It is for checkAndPut and checkAndDelete.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 159
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159>
bq. >
bq. > 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?
bq. >
bq. > 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?
Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to
have simple interface instead, right? That means we may have to have too many
methods. For java interface, it is ok and preferred to have many methods. But
for RPC, I think it is better to have fewer and flexible methods.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 193
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>
bq. >
bq. > What is this implementing from current Delete? The delete family?
bq. >
bq. > This is returned to the client?
bq. >
bq. > How do we do a column that has no qualifier? Thats possible in
hbase.
To delete a family, you don't specify the qualifier. To delete a only one
version of a column, you sepcify the family, qualifer, timestamp, and set
oneVersion to true.
To delete all version of a column, you specify the family, qualifier
What do you mean a column that has no qualifier? Are you referring to
Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 214
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>
bq. >
bq. > Yeah, why have regionspecified in the mutate if you are going to
provide it as a param too?
The same reason as for Get. It is used as a default region in case all/most
mutates are for a same region.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 233
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233>
bq. >
bq. > Why this when regions can change. Also, a Scan can traverse many
regions so what is the regionspecifier referring to? The first?
Yes, it is the first region.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 276
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line276>
bq. >
bq. > Is this supposed to be row specifier?
It is the region where the row is on. The row is in line 277.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 286
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line286>
bq. >
bq. > ditto
same as LockRowRequest.
bq. On 2012-03-26 23:21:58, Michael Stack wrote:
bq. > src/main/proto/RegionClient.proto, line 338
bq. > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338>
bq. >
bq. > Does this belong here? You provide it as a param when making the
request.
Yes, this is for Region protocol, not client API. You need to specify for
which region this action will be executed on, right?
- Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review6365
-----------------------------------------------------------
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
