[
https://issues.apache.org/jira/browse/HBASE-5443?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220823#comment-13220823
]
[email protected] 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
