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 Fri, 02 Mar 2012 10:31:00 GMT

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

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


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


This seems to be close to a one-to-one mapping with the current interface today.  I don't
know if this is the intent or whether you're willing to completely redesign the look of the
API too.  Maybe it's to ease the transition?

I'd like to see a request type to do one-shot scans.  Something where you don't even get a
scanner ID.  You pass parameters like to open a scanner, you say up to how many rows or bytes
you want to retrieve, and you get just that in one shot.
When opening a actual scanner, we also need to be able to get the first batch of scan results
at the same time we open the scanner.  This is a must-have IMO.  And we need to be able to
request to close the scanner while fetching a batch of results.

It would be nice to have a "keep-alive" request for existing scanners.  Something to tell
the server "I'm not fetching anything from this scanner right now, but please keep it open
by reseting its TTL, don't close it just because I haven't used it for a while".

Please, please, please, consider shortening the name of all these protobufs and dropping the
Proto suffix.  The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily"
for something that describes the multiple things you're trying to get out of a row) or are
too redundant (e.g. "KeyType keyType").

Regarding the lack of "multi" RPC, I think this is a good thing.  "multi" is a big mess that
was only marginally better than its horrible "multiPut" predecessor.  This proposal already
supports multi-everything, it just doesn't support batching different kind of operations in
the same RPC, which isn't a big deal IMO.


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

    Do this instead:
    
      if which cygpath >/dev/null 2>/dev/null; then
        # Windows
      else
        # Not Windows
      fi



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

    Simply do:
    
      if $IS_WIN; then



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

    Actually you can just remove the whole $IS_WIN business and everything.  Simply fix PROTO_DIR
and JAVA_DIR when on Windows before calling protoc.



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

    I find the Proto suffix unnecessary and long.  If you truly want a suffix, PB would be
shorter, but no suffix would be better IMO.



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

    Use "option optimize_for = SPEED", it makes a big difference.



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

    I'd call this just "Columns".



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

    I would recommend to pluralize all "repeated" fields.  This will make for nicer code where
you'll be able to write something along the lines of:
    
      for (byte[] qualifier : pb.qualifiers())



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

    The thing that append() and increment() have in common is that they're atomic operations
that don't require a read-modify-write from the client.  So maybe AtomicOp would be better?



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

    Just call this Columns.



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

    What's the meaning of this?  How do we know what has been processed and what hasn't?



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

    I'd vote for adding this right now.  It's easy to add directly and would be a huge improvement
for short scans (which are super common).



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

    We need to have a way to get feedback from the server about the TTL of the scanner.  How
long can the client hold on to the scanner before the server will kill it.  Add a field here
so that the server can communicate the TTL to the client.



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

    Please add an "optional boolean close" to request that the scanner be closed after returning
this batch of results.  This can help clients eliminate the "CloseScannerRequestProto" when
they know they're going to close the scanner after this batch.



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

    This name is inconsistent with the name of the request.  By your scheme it should be named
"LockRowResponseProto" – although I'd much prefer "LockResp" or something like that.



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

    This needs to have a field specifying how long the server is willing to hand out the lock
for.



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

    Call these fields just "from" and "to".



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

    Why are all these fields optional?  How can a KeyValue not have a family or a qualifier
or a timestamp?



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

    I'd recommend naming this "timestamp" not "time".



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

    Just call this "type".


- Benoit


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