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 06:01:45 GMT

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

jiraposter@reviews.apache.org commented on HBASE-5619:
------------------------------------------------------



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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      There is already an exiting folder called protobuf (rest).  Let me change the dir
holds the .proto files under src/main to protobuf.

good


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.

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

I'd be fine w/ the doc being sparse but there are a few cases where doc is necessary; e.g.
the one I cite above.


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.

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.  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?
bq.  
bq.  Jimmy Xiang wrote:
bq.      Some call expects a region name, some call expects an encoded region name.  With
a specifier, we can handle both.
bq.      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.
bq.      
bq.      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.
bq.      
bq.      I will add a comment for it.

ok


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.

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

So you are doing this so you have to create less objects?  So you can avoid MultiResponse
with its regionname to responses?


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

Needs comment


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.

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?


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?

On 'Any problem with that?', no.  Just trying to avoid redundancy that makes the Interface
imprecise and therefore hard to grok.


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.

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)


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

'But for RPC, I think it is better to have fewer and flexible methods.'  Fair enough.  Would
like to make it just flexible enough and no more (smile).  Needs documentation too else fellas
like me will come in and get wrong impression on whats going on.


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?

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

Having 'default' region doesn't sound right, even if you are trying to be flexible.


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.

You know what I am going to say (smile)


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?

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.  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.
bq.  
bq.  Jimmy Xiang wrote:
bq.      It is still there.  It is collapsed into the Get call.

ok.  good.


- 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