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 Mon, 26 Mar 2012 23:22:31 GMT

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

jiraposter@reviews.apache.org 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

        

Mime
View raw message