[
https://issues.apache.org/jira/browse/HBASE-5443?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13218706#comment-13218706
]
jirapos...@reviews.apache.org commented on HBASE-5443:
------------------------------------------------------
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > 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?
Yes. Todd mentioned that too. Will do.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 36
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36>
bq. >
bq. > My guess is that there is no uint32?
There is uint32. Should we use uint32 for timestamp? If it is in second, it is
ok for many years. If it is in millisecond, we should use uint64.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 42
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line42>
bq. >
bq. > Whats a 'name'? Is it a region name?
It is for a generic NameValue pair.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > pom.xml, line 749
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line749>
bq. >
bq. > Do we do this elsewhere in the pom? If so, should set a boolean
once?
This is the only place.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 1
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line1>
bq. >
bq. > 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.....)
All proto files should go here, while Java files go to other places like other
Java classes.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 21
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line21>
bq. >
bq. > 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?
Sure, will do.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 22
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22>
bq. >
bq. > Should the suffix be Proto -- our convention for Protobuf classes
(?) -- or Protos? Why the plural? Perhaps this a container for a bunch of
Proto?
Yes, there are a bunch of messages. Each message is a Proto.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 23
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line23>
bq. >
bq. > This class seems to have more than HR stuff in it. Should we make
more protos? A client proto?
Ok.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 62
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line62>
bq. >
bq. > What is this? For filters?
For checkAndPut, checkAndDelete, this is condition to check.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 88
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>
bq. >
bq. > Mutate is not deprecated? We don't have deprecated stuff in this
proto file? This is doing what from current API?
This Mutate is different from the mutateRow() in HRegionInterface. It is for
append() and increment(). Any suggestion for a better naming?
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 105
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line105>
bq. >
bq. > Are these client datastructures? Or they go w/ the RS proto and
are used by clients?
For now, I prefer not to change the client data structures. So these go w/ the
RS proto only and are not used by clients directly.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 118
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line118>
bq. >
bq. > Where does this come from in current API?
Yes.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 148
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line148>
bq. >
bq. > WALEntryProto?
Ok.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 126
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line126>
bq. >
bq. > Doesn't HRI have a tablename in it? So maybe this should have a HRI?
bq. >
bq. > What is this LogKeyProto modeling? Should be WALKeyProto?
WAL log key. Will change the name.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 162
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line162>
bq. >
bq. > Should there be more in here?
It should be bytes. I will it.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 165
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line165>
bq. >
bq. > Is this a class or method name? If method name is convention to
capitalize?
It is the request class.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 169
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line169>
bq. >
bq. > So, even if method returns a HRegionInfo, we return a
GetRegionInfoResponseProto in case the response changes?
That's right. The response proto contains the HRegionInfo in this case.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 174
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line174>
bq. >
bq. > regionName and encodedName and HRegionInfo class.... should we
standardize?
Yes, working on it.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 223
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line223>
bq. >
bq. > Maybe NextOnScannerRequestProto so relates better to our current API
Sure. Will rename it.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 248
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line248>
bq. >
bq. > Man. Anyone use this thing?
Could not find any reference. Should we deprecate it?
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 408
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line408>
bq. >
bq. > nextOnScanner?
Will rename it.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 420
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line420>
bq. >
bq. > These are admin interface functions. Should we split the interface?
Yes, working on it now.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 62
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62>
bq. >
bq. > Should this be uint32? (Maybe not possible in proto?)
Is it better to use uint64? I assume it is in millisecond.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 65
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line65>
bq. >
bq. > 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.
bq. >
bq. > Should ServerName be in this proto file?
Yes, ServerName should be in this file.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/hbase.proto, line 33
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line33>
bq. >
bq. > Doesn't an HRI have a Map?
bq. >
bq. > There does not seem to be enough in here.
I did not in any map in HRegionInfo. Anything specific I missed?
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 464
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line464>
bq. >
bq. > Yeah, we need to doc. this later. Ain't the doc here going to be
our Interface doc? Our only Interface doc?
Right. Doc comes later.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 399
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line399>
bq. >
bq. > I wonder if these could take a more primitive type... a KV type.
Then we may have problem to enhance it, for example, add/remove a parameter.
In this case, we can always add/deprecate optional parameters to the Request.
bq. On 2012-02-28 20:44:26, Michael Stack wrote:
bq. > src/main/proto/HRegionProtocol.proto, line 315
bq. > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line315>
bq. >
bq. > We should have one or the other. Can have one call through to the
other (thats implementation?)
Will collapse closeRegionByName into closeRegion.
- Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4054/#review5407
-----------------------------------------------------------
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
