hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5619) Create PB protocols for HRegionInterface
Date Tue, 27 Mar 2012 19:55:30 GMT

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

jiraposter@reviews.apache.org 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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      I see.  I will move it out to the request so it is easier to understand.

I think this would be good.


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)
bq.  
bq.  Jimmy Xiang wrote:
bq.      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.

My guess is that this region in a Scan won't do you any good.  The client figures where the
Scan is at any one time and what region it should be on.  There is probably no value having
this in the Scan.


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      I used to put these transient parameters in the protobuff RegionInfo as well.  However
Todd thought it's better to put them outside.
bq.      
bq.      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.  
bq.  Michael Stack wrote:
bq.      Hmm.  Moving out these flags changes the current 'model' but in a direction we should
be headed in.  The split/offline stuff were stuffed into HRI in the first place just because
this was an easy way to pass these transient states around in hbase; they also are less important
now in HRI though still depended on when we scan meta IIRC.  Its probably better to evolve
toward an HRI that is immutable once made.  So I'd be down w/ moving them out but its up to
you.  It might be easier on you achieving parity w/ first commit of pb work if the pb classes
are like the internals they will be feeding into.
bq.  
bq.  Jimmy Xiang wrote:
bq.      Ok, I can move them back.  We can get back to this when time comes.

That might be easiest.  I think yeah, they don't really belong in here and we've made some
work undoing their need to be i here but lets do that in another issue.... 


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      I was thinking to set it if the HRegion.flushCache call returned true.  It is just
informational.
bq.  
bq.  Michael Stack wrote:
bq.      Does that imply a synchronous call?  I thought flushCache just queued the flush then
returned to the client w/o actually waiting on flush to complete.
bq.  
bq.  Jimmy Xiang wrote:
bq.      I checked the code and it seems that flushCache is a synchronous call.  Should we
make it async?  If so, I can make flushed to queued.

Then I was wrong in my supposition.  Lets do what you suggest, an actual return with a response
w/ some value in 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?
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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      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?

I think we do what is currently there first and then after pb goes in, work on changing it.
 This extra flag in Get is fine (Check exists could be done as a filter if we were going to
redo).   Lets add it to make Get flexible as you suggest we do elsewhere in these remarks.


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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      Right.  Should I create a separate message for MultiResponse?

On 'Should I create a separate message for MultiResponse?', I defer to you.   You have your
head in this stuff.   It sounds cleaner to me having the Mutate being one message and then
Mutate+regionspec be a new Message that is used trying to do Multi stuff but you are closer
to what is going on here.


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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      I see.  I will remove the region from Get. It is ok not to support multiGet, right?

We should support multiget, yes.  Do you have to make a new message to do that if you remove
the regionspec from Get?  If so, maybe that is ok?  If not, i'm fine w/ it as optional regionspec
in Get.  I think it would be fine to purge regionspec from the Get, Put, Delete, objects and
then add messages to support multactions.  Up to you.  Just doc it and be consistent (smile).


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).
bq.  
bq.  Jimmy Xiang wrote:
bq.      In this case, all columns in the family for this row will be deleted, right?

IIRC, yes.  See http://hbase.apache.org/book.html#delete where I tried to doc the options


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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      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
bq.      call to handle RowMutations in the same call.  I can try to handle RowMutations in
the multi call.
bq.      
bq.      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?

On "I can try to handle RowMutations in the multi call.", good.   So, we won't do multiple
calls to do many?


- Michael


-----------------------------------------------------------
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

        

Mime
View raw message