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-5443) Add PB-based calls to HRegionInterface
Date Tue, 28 Feb 2012 20:45:46 GMT

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

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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review5407
-----------------------------------------------------------


High-level, do we need to split the Interface more -- into admin vs user protos?   Would love
to get rid of the plethora of methods but probably not a task to be done as part of this issue?


pom.xml
<https://reviews.apache.org/r/4054/#comment11742>

    There are prereqs for this to work?  I suppose compile-proto.sh checks and its failing
it seems fails the build.. thats good.



pom.xml
<https://reviews.apache.org/r/4054/#comment11741>

    Do we do this elsewhere in the pom?  If so, should set a boolean once?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11743>

    So, all protobuf files go here?  We need to ensure this the case w/ all patches (I think
the rpc patch was putting them elsewhere.....)



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11744>

    Elsewhere in hbase the convention is 'what' and then the generated classes are in a 'generated'
subpackage as in there is a thrift subpackage and under there are thrift utils and then a
'generated' subpackage.  Ditto avro.  This is different in that we have a generated top level
subpackage w/ proto as a subpackage.  Should we flip it?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11745>

    Should the suffix be Proto -- our convention for Protobuf classes (?) -- or Protos?  Why
the plural?  Perhaps this a container for a bunch of Proto?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11755>

    This class seems to have more than HR stuff in it.  Should we make more protos?  A client
proto?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11751>

    Mutation (Probably already used)



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11752>

    What is this?  For filters?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11753>

    Mutate is not deprecated?  We don't have deprecated stuff in this proto file?  This is
doing what from current API?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11756>

    Are these client datastructures?   Or they go w/ the RS proto and are used by clients?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11754>

    Where does this come from in current API?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11758>

    Doesn't HRI have a tablename in it?  So maybe this should have a HRI?
    
    What is this LogKeyProto modeling?  Should be WALKeyProto?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11759>

    WALEntryProto?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11760>

    Should there be more in here?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11761>

    Is this a class or method name?  If method name is convention to capitalize?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11762>

    So, even if method returns a HRegionInfo, we return a GetRegionInfoResponseProto in case
the response changes?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11763>

    regionName and encodedName and HRegionInfo class.... should we standardize?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11764>

    Excellent.  Later we need to return first set of results in here rather than have to have
client go back to do a next.



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11765>

    Maybe NextOnScannerRequestProto so relates better to our current API



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11766>

    We should make this optional some day.. that you have to call a close.



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11767>

    Man.  Anyone use this thing?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11768>

    We should have one or the other.  Can have one call through to the other (thats implementation?)



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11769>

    Oh, I see how method naming and classes goes



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11770>

    I wonder if these could take a more primitive type... a KV type.



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11771>

    nextOnScanner?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11772>

    These are admin interface functions.  Should we split the interface?



src/main/proto/HRegionProtocol.proto
<https://reviews.apache.org/r/4054/#comment11773>

    Yeah, we need to doc. this later.  Ain't the doc here going to be our Interface doc? 
Our only Interface doc?



src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11750>

    Doesn't an HRI have a Map?
    
    There does not seem to be enough in here.



src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11748>

    My guess is that there is no uint32?



src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11746>

    Whats a 'name'?  Is it a region name?



src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11747>

    Should this be uint32?  (Maybe not possible in proto?)



src/main/proto/hbase.proto
<https://reviews.apache.org/r/4054/#comment11749>

    I think a bit of doc in here would not go amiss.  Else its hard to figure all is going
on in here; what goes where.
    
    Should ServerName be in this proto file?


- Michael


On 2012-02-27 18:54:31, 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-02-27 18:54:31)
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-5443.
bq.      https://issues.apache.org/jira/browse/HBASE-5443
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    pom.xml 066c027 
bq.    src/main/proto/HRegionProtocol.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.


                
> Add PB-based calls to HRegionInterface
> --------------------------------------
>
>                 Key: HBASE-5443
>                 URL: https://issues.apache.org/jira/browse/HBASE-5443
>             Project: HBase
>          Issue Type: Sub-task
>          Components: ipc, master, migration, regionserver
>            Reporter: Todd Lipcon
>            Assignee: Jimmy Xiang
>             Fix For: 0.96.0
>
>         Attachments: region_java-proto-mapping.pdf
>
>


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