[ 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