[ https://issues.apache.org/jira/browse/HBASE-5619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13236880#comment-13236880 ]
jirapos...@reviews.apache.org commented on HBASE-5619: ------------------------------------------------------ bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > Looks good overall. bq. > Minor comments below. Thanks for reviewing. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionAdmin.proto, line 115 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line115> bq. > bq. > Is it possible that clusterIdLeastSignificantBits is present but clusterIdMostSignificantBits is absent ? They both should be either there, or not. I can add an optional message UUID, which should have both listSigBits and mostSigBits. How is that? bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 147 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line147> bq. > bq. > Remove 'is' Removed bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 153 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line153> bq. > bq. > white space. Removed. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionAdmin.proto, line 171 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line171> bq. > bq. > Should this rpc be called getOnlineRegions ? For repeated parameter, for example, region, the generated method is something like getRegionList(). So it may be better to use singular for parameters. For similar reason, I did the same for method name. Another reason for that is, in the future, we may use this method to just get one online region given a region name. This way, we can limit the number of methods. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 139 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line139> bq. > bq. > Increment doesn't extend Mutation: bq. > bq. > public class Increment implements Row { bq. > That's right. But we can still make increment a mutation, in protocol buffer. Since Increment doesn't extend Mutation, it is a little bit different to map an Increment to a Mutate. So we don't have to have a separate method for Increment. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 171 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line171> bq. > bq. > Should this line be removed ? Increment is treated as a Mutate, so it is needed. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 208 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line208> bq. > bq. > 'it's appeared' -> 'it appears' Fixed. bq. On 2012-03-23 17:54:17, Ted Yu wrote: bq. > src/main/proto/RegionClient.proto, line 335 bq. > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line335> bq. > bq. > I don't see region member in Exec.java bq. > Do we need this ? I see. Let me enhance the doc. The reason for that is Exec is called for a region. In the request level, there is a default region. Users can specify different region for each individual Exec. - Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6294 ----------------------------------------------------------- On 2012-03-22 20:20:39, 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-03-22 20:20:39) 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-5619. bq. https://issues.apache.org/jira/browse/HBASE-5619 bq. bq. bq. Diffs bq. ----- bq. bq. pom.xml 10b13ef bq. src/main/proto/RegionAdmin.proto PRE-CREATION bq. src/main/proto/RegionClient.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. > Create PB protocols for HRegionInterface > ---------------------------------------- > > Key: HBASE-5619 > URL: https://issues.apache.org/jira/browse/HBASE-5619 > Project: HBase > Issue Type: Sub-task > Components: ipc, master, migration, regionserver > Reporter: Jimmy Xiang > Assignee: Jimmy Xiang > Fix For: 0.96.0 > > Attachments: hbase-5619.patch > > > Subtask of HBase-5443, separate HRegionInterface into admin protocol and > client protocol, create the PB protocol buffer files -- 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