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

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


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


I notice you followed the same declaration order as HRegionInterface.  Could we 
reorder these along say, functionality lines with appropriate comments in the 
.proto file?
NOTE: I see Todd suggested splitting along management/client lines.  That may 
be superior.  Here's a sketch of what I think "along functionality lines" would 
look like:

// Row(s) level
get
put
delete
mutate
openScanner
fetchFromScanner
closeScanner
lockRow
unlockRow

// Region level info
getRegionInfo
getOnlineRegions
getLastFlushTime
getStoreFileList

// Region-level mutation
flushRegion
openRegion
closeRegion
closeRegionByEncodedName
splitRegion
compactRegion
bulkLoadHFiles
execCoprocessor

// RegionServer-level
getBlockCacheColumnFamilySummaries
replicateLogEntry
rollLogWriter
stopServer



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

    First enum is missing a comment.  I might just get rid of the comments 
though, I don't think they really add anything.



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

    The name mutate doesn't seem consistent to me.  For one, "mutate" seems to 
not support Put/Deletes, but RowMutation *only* seems to support Puts/Deletes.  
Perhaps we can collapse this somehow?



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

    this should be heapSize.



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

    You got rid of mutateRow and suggest the new put/delete can support as 
along as there are no mixed puts and deletes.
    
    But it looks like mutateRow allowed you to do mixed puts and deletes 
atomically.  Could you support atomic puts and deletes with what you have here?


- Gregory


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

        

Reply via email to