[
https://issues.apache.org/jira/browse/HBASE-5443?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221119#comment-13221119
]
jirapos...@reviews.apache.org commented on HBASE-5443:
------------------------------------------------------
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > 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?
bq. >
bq. > 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.
bq. > 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.
bq. >
bq. > 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".
bq. >
bq. > 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").
bq. >
bq. > 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.
bq.
bq. Michael Stack wrote:
bq. We should implement what Benoît is asking for, probably not all as
part of this issue. That said, if possible can we try and accomodate what he's
asking for down here at the rpc level? I suppose once all is pb, it should be
easy enough adding new stuff but it would be good to keep in mind what he's
asking while redoing this layer. In a later issue we can add the overloads
that exploit the additions or add the new methods B wants (What B is asking for
are long-time outstanding fixups needed in hbase). For example, can the pb
response on open of a scanner be more than just the scanner id; could it
include an optional result item? Or I suppose, once up on pb, we can do this
easily enough later?
bq.
bq.
The idea is not to break the existing client application code. So the new
interface should be able to do the same thing and more.
By the way, I have changed the interfaces a lot after several reviews so I
closed this review. I will post a new review later.
As to scanner, we cannot retrieve everything in one shot. So in the RPC layer,
there must be multiple trips. As to the function you mentioned, it can be
built in the client side, right?
I will add an option to return results in opening a scanner, and an option to
close the scanner in fetching from the scanner.
Ok, I will try to shorten the names.
As to multi, I am not sure. This proposal doesn't support mix different kind of
operations in different order in the same RPC. I may need to add a similar one
if we don't want to break the existing function.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 22
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22>
bq. >
bq. > 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.
Ok, I will remove the Proto suffix.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 25
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line25>
bq. >
bq. > Use "option optimize_for = SPEED", it makes a big difference.
Cool. I will add it.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 28
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line28>
bq. >
bq. > I'd call this just "Columns".
I changed it to ColumnProto.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 30
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line30>
bq. >
bq. > 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:
bq. >
bq. > for (byte[] qualifier : pb.qualifiers())
For the repeated fields, the generated code will have method
addQualifier(index), getQualifierList(). I don't think we need pluralization.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 88
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>
bq. >
bq. > 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?
In my new one, I have an optional parameter to indicate if atomic operations
requested, which is for a set of mutations.
One mutation such as append() and increment() should always be atomic, right?
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 192
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line192>
bq. >
bq. > What's the meaning of this? How do we know what has been processed
and what hasn't?
I removed it.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 221
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line221>
bq. >
bq. > 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.
Sure. Will add it.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 226
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line226>
bq. >
bq. > 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.
Sure, will do.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 269
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line269>
bq. >
bq. > 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.
How about LockRowResponse?
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 271
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line271>
bq. >
bq. > This needs to have a field specifying how long the server is willing
to hand out the lock for.
Ok, will add one.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/hbase.proto, line 36
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36>
bq. >
bq. > Call these fields just "from" and "to".
Ok.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/hbase.proto, line 60
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line60>
bq. >
bq. > Why are all these fields optional? How can a KeyValue not have a
family or a qualifier or a timestamp?
In case later on we replace the existing KeyValue class with this one.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/hbase.proto, line 62
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62>
bq. >
bq. > I'd recommend naming this "timestamp" not "time".
ok.
bq. On 2012-03-02 10:30:06, Benoit Sigoure wrote:
bq. > src/main/proto/hbase.proto, line 63
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line63>
bq. >
bq. > Just call this "type".
If later on, we add another type, such as ValueType, it will be confusing,
right?
- Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review5552
-----------------------------------------------------------
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
