[ 
https://issues.apache.org/jira/browse/HBASE-5619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239733#comment-13239733
 ] 

[email protected] commented on HBASE-5619:
------------------------------------------------------



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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      Yes, this is for Region protocol, not client API.  You need to specify 
for which region this action will be executed on, right?
bq.  
bq.  Michael Stack wrote:
bq.      Yes.  I got into habit of complaining about the region specs. They 
make sense when they are params.  Its regions internal to Get, etc., that 
causes me difficulty.

I see.  I will move it out to the request so it is easier to understand.


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      Yes, it is the first region.
bq.  
bq.  Michael Stack wrote:
bq.      You know what I am going to say (smile)

Right. Shall I rename it to startRegion, or move it out, or just get rid of it 
and figure out the region dynamically based on the startRow?


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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.
bq.      To delete all version of a column, you specify the family, qualifier
bq.      
bq.      What do you mean a column that has no qualifier? Are you referring to 
Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?
bq.  
bq.  Michael Stack wrote:
bq.      Yeah.  You can put a value at row, family, null qualifier, ts. You can 
also delete it (might not be a good idea but you can do it).

In this case, all columns in the family for this row will be deleted, right?


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.  
bq.  Michael Stack wrote:
bq.      Could we just have it out here as a param on the get and not in the 
Get object?  Will that work (I think I understand why Result needs it internal 
to cut down on objects we need to support multiresponses)

Sure, I can do that for get.


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      The interface is kind of similar, but very flexible. Any problem with 
that?
bq.  
bq.  Michael Stack wrote:
bq.      On 'Any problem with that?', no.  Just trying to avoid redundancy that 
makes the Interface imprecise and therefore hard to grok.

I see.  I will remove the region from Get. It is ok not to support multiGet, 
right?


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.  
bq.  Michael Stack wrote:
bq.      I see specifying the region name in this GetRequest but not sure about 
why we'd have region in the Get object itself if we are doing it here on the 
invocation.  Seems like the two contend?

I will remove it from Get object to avoid confusion for now.


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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.
bq.      In this case, I'd like the server to return the region name.  This is 
optional.  In most call, it is not used.
bq.  
bq.  Michael Stack wrote:
bq.      So you are doing this so you have to create less objects?  So you can 
avoid MultiResponse with its regionname to responses?

Right.  Should I create a separate message for MultiResponse?


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.  
bq.  Michael Stack wrote:
bq.      The current implementation actually does the fetch and in the client 
checks it null or not IIRC?  Or is it all serverside?  So you add this to the 
Get so you don't have to do the exists call?   You can implement it w/ the 
addition to Get?

It is all in the server side. I can do it in the client side and remove it.  
That means some wasted network bandwidth.  What do you think?


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      The same reason as for Get.  It is used as a default region in case 
all/most mutates are for a same region.
bq.  
bq.  Michael Stack wrote:
bq.      Having 'default' region doesn't sound right, even if you are trying to 
be flexible.

I see. For get, we put the region in the request level.  I prefer to do the 
same for mutate.  But if we put the region in request for mutate, we have to 
have a several
call to handle RowMutations in the same call.  I can try to handle RowMutations 
in the multi call.

Another thing is that, if we put the region in the request level, do we need to 
support multiple Get per get request?  How about multiple Mutate per mutate 
request?


- 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

        

Reply via email to