Details

    • Hadoop Flags:
      Reviewed

      Description

      Subtask of HBase-5443, separate HRegionInterface into admin protocol and client protocol, create the PB protocol buffer files

      1. hbase-5619.patch
        21 kB
        Jimmy Xiang
      2. hbase-5619_v3.patch
        24 kB
        Jimmy Xiang
      3. hbase-5619_v4.patch
        24 kB
        Jimmy Xiang
      4. hbase-5619_v5.patch
        1.62 MB
        Jimmy Xiang
      5. 5619v6.txt
        1.63 MB
        stack
      6. 5619v6.txt
        1.63 MB
        stack

        Activity

        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        Hide
        stack added a comment -

        I tried TestFromClientSide a few times locally and its passing fine.

        Show
        stack added a comment - I tried TestFromClientSide a few times locally and its passing fine.
        Hide
        stack added a comment -

        Committed to trunk (hadoopqa doesn't seem to be picking this up... can fixup afterward if an issue). Thanks for the patch Jimmy.

        Show
        stack added a comment - Committed to trunk (hadoopqa doesn't seem to be picking this up... can fixup afterward if an issue). Thanks for the patch Jimmy.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520665/5619v6.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 70 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestFromClientSide
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12520665/5619v6.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 70 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestFromClientSide org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1354//console This message is automatically generated.
        Hide
        Jimmy Xiang added a comment -

        @Stack, thanks!

        Show
        Jimmy Xiang added a comment - @Stack, thanks!
        Hide
        stack added a comment -

        Reattach to see if it triggers hadoopqa build

        Show
        stack added a comment - Reattach to see if it triggers hadoopqa build
        Hide
        stack added a comment -

        Let me try it against hadoopqa

        Show
        stack added a comment - Let me try it against hadoopqa
        Hide
        stack added a comment -

        This is Jimmy's patch with a package.html added at o.a.h.h.protobuf to say whats going on in the package and then a README at src/main/protobuf with directions and a bit of script (from pom.xml amended) on how to regen the classes if you make changes to definition files.

        I tried to find a way to add an apache license to the generated files – its probably not necessary... they have headers which say they are generated files – and a bit more doc on where the proto files are, etc., but I couldn't figure how from reading protobuf documentation.

        Also excluded new generated files from findbugs report

        Show
        stack added a comment - This is Jimmy's patch with a package.html added at o.a.h.h.protobuf to say whats going on in the package and then a README at src/main/protobuf with directions and a bit of script (from pom.xml amended) on how to regen the classes if you make changes to definition files. I tried to find a way to add an apache license to the generated files – its probably not necessary... they have headers which say they are generated files – and a bit more doc on where the proto files are, etc., but I couldn't figure how from reading protobuf documentation. Also excluded new generated files from findbugs report
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520550/hbase-5619_v5.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 68 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12520550/hbase-5619_v5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 68 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1350//console This message is automatically generated.
        Hide
        Jimmy Xiang added a comment -

        I reverted the pom file change, and checked in the generated protobuf classes.

        Show
        Jimmy Xiang added a comment - I reverted the pom file change, and checked in the generated protobuf classes.
        Hide
        Jimmy Xiang added a comment -

        That's what do for thrift now, not avro.

        Show
        Jimmy Xiang added a comment - That's what do for thrift now, not avro.
        Hide
        stack added a comment -

        I like the idea of checking in the generated protobufs files. They'll change rarely. Let the fella who generates them have to worry about his protoc setup (This is what we do now for thrift and avro?)

        Show
        stack added a comment - I like the idea of checking in the generated protobufs files. They'll change rarely. Let the fella who generates them have to worry about his protoc setup (This is what we do now for thrift and avro?)
        Hide
        Enis Soztutar added a comment -

        I was suggesting adding the generated files under src/, and adding the runtime protobuf dependency in maven. But if it won't work, we can go with requiring protoc. Dynamic installation will be a little bit overkill.

        Show
        Enis Soztutar added a comment - I was suggesting adding the generated files under src/, and adding the runtime protobuf dependency in maven. But if it won't work, we can go with requiring protoc. Dynamic installation will be a little bit overkill.
        Hide
        Jimmy Xiang added a comment -

        But for proto files, other projects depend on protoc, for example HADOOP/HDFS. We are moving towards pb, protoc dependency should be fine.
        I can try to setup a temp protoc dynamically.

        Show
        Jimmy Xiang added a comment - But for proto files, other projects depend on protoc, for example HADOOP/HDFS. We are moving towards pb, protoc dependency should be fine. I can try to setup a temp protoc dynamically.
        Hide
        Enis Soztutar added a comment -

        I think for compiling protoc installation should not be a dependency. Why don't we add the generated java files to the source tree, so that only if you change the protoc definitions, you would have to recompile the .proto files. As far as I know, this is how it is done for thrift dependencies in other projects.

        Show
        Enis Soztutar added a comment - I think for compiling protoc installation should not be a dependency. Why don't we add the generated java files to the source tree, so that only if you change the protoc definitions, you would have to recompile the .proto files. As far as I know, this is how it is done for thrift dependencies in other projects.
        Hide
        Jimmy Xiang added a comment -

        @Stack, so far, I could not find a good protoc maven plugin. I don't remember I tried to install it on my Ubuntu.
        That's the download site for protobuf compiler: http://code.google.com/p/protobuf/downloads/list
        But for Linux, I think it is easy to install with rpm/apt-get.

        Show
        Jimmy Xiang added a comment - @Stack, so far, I could not find a good protoc maven plugin. I don't remember I tried to install it on my Ubuntu. That's the download site for protobuf compiler: http://code.google.com/p/protobuf/downloads/list But for Linux, I think it is easy to install with rpm/apt-get.
        Hide
        stack added a comment -

        @Jimmy Is there a maven plugin for protoc so folks don't have to install it? At least, we should be checking for its presence and complaining w/ a pointer to a how-to-install doc if not present? Is that possible?

        Show
        stack added a comment - @Jimmy Is there a maven plugin for protoc so folks don't have to install it? At least, we should be checking for its presence and complaining w/ a pointer to a how-to-install doc if not present? Is that possible?
        Hide
        Jimmy Xiang added a comment -

        @Stack, could you please install protoc and give it a try again?
        From now on, we need protoc to compile.

        Show
        Jimmy Xiang added a comment - @Stack, could you please install protoc and give it a try again? From now on, we need protoc to compile.
        Hide
        Ted Yu added a comment -

        @Stack:
        Was protoc installed on the box you performed the build.

        I did a clean build after applying patch v4 and didn't see the above error.

        Show
        Ted Yu added a comment - @Stack: Was protoc installed on the box you performed the build. I did a clean build after applying patch v4 and didn't see the above error.
        Hide
        stack added a comment -

        Can you fix this Jimmy? Happens when I try to build after applying the patch:

        main:
             [exec] target/compile-proto.sh: line 12: protoc: command not found
             [exec] target/compile-proto.sh: line 12: protoc: command not found
             [exec] target/compile-proto.sh: line 12: protoc: command not found
        [INFO] ------------------------------------------------------------------------
        [ERROR] BUILD ERROR
        [INFO] ------------------------------------------------------------------------
        [INFO] An Ant BuildException has occured: exec returned: 127
        
        Show
        stack added a comment - Can you fix this Jimmy? Happens when I try to build after applying the patch: main: [exec] target/compile-proto.sh: line 12: protoc: command not found [exec] target/compile-proto.sh: line 12: protoc: command not found [exec] target/compile-proto.sh: line 12: protoc: command not found [INFO] ------------------------------------------------------------------------ [ERROR] BUILD ERROR [INFO] ------------------------------------------------------------------------ [INFO] An Ant BuildException has occured: exec returned: 127
        Hide
        Jimmy Xiang added a comment -

        @Stack, could you please commit this patch? I do have some changes to pb files. But I'd like to address them in HBASE-5620.

        Thanks.

        Show
        Jimmy Xiang added a comment - @Stack, could you please commit this patch? I do have some changes to pb files. But I'd like to address them in HBASE-5620 . Thanks.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520200/hbase-5619_v4.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 73 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.replication.TestReplication
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12520200/hbase-5619_v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 73 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.replication.TestReplication org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1323//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6453
        -----------------------------------------------------------

        Ship it!

        I'm game for committing this. My guess is that it'll need some massage before we arrive at final for 0.96. Will let this bake a day or so before making actual commit in case someone else wants to take a looksee. Nice stuff Jimmy

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14078>

        This seems off.

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14079>

        Great

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14080>

        Great

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14081>

        oh, here is getclosestrowbefore. good. easy to remove.

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14082>

        Does this break if qualifier is null? It should work?

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14083>

        This modeling seems a little odd. I'd think that a Mutate would have a column and then a ts and a value but lets leave it this way. My guess is that you've figured that this will make your life easier going forward (if so, lets keep this 'model'...)

        src/main/protobuf/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment14085>

        So, can an open scanner return results? Curently returns scanid only. This looks like it could return scanid AND first set of results. That'd be cool.

        • Michael

        On 2012-03-27 21:57:06, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-27 21:57:06)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/protobuf/RegionAdmin.proto PRE-CREATION

        src/main/protobuf/RegionClient.proto PRE-CREATION

        src/main/protobuf/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6453 ----------------------------------------------------------- Ship it! I'm game for committing this. My guess is that it'll need some massage before we arrive at final for 0.96. Will let this bake a day or so before making actual commit in case someone else wants to take a looksee. Nice stuff Jimmy src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14078 > This seems off. src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14079 > Great src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14080 > Great src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14081 > oh, here is getclosestrowbefore. good. easy to remove. src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14082 > Does this break if qualifier is null? It should work? src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14083 > This modeling seems a little odd. I'd think that a Mutate would have a column and then a ts and a value but lets leave it this way. My guess is that you've figured that this will make your life easier going forward (if so, lets keep this 'model'...) src/main/protobuf/RegionClient.proto < https://reviews.apache.org/r/4054/#comment14085 > So, can an open scanner return results? Curently returns scanid only. This looks like it could return scanid AND first set of results. That'd be cool. Michael On 2012-03-27 21:57:06, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-27 21:57:06) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/protobuf/RegionAdmin.proto PRE-CREATION src/main/protobuf/RegionClient.proto PRE-CREATION src/main/protobuf/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/
        -----------------------------------------------------------

        (Updated 2012-03-27 21:57:06.565352)

        Review request for hbase.

        Changes
        -------

        Addressed Stack's comments.

        Summary
        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.
        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs (updated)


        pom.xml 10b13ef
        src/main/protobuf/RegionAdmin.proto PRE-CREATION
        src/main/protobuf/RegionClient.proto PRE-CREATION
        src/main/protobuf/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-27 21:57:06.565352) Review request for hbase. Changes ------- Addressed Stack's comments. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs (updated) pom.xml 10b13ef src/main/protobuf/RegionAdmin.proto PRE-CREATION src/main/protobuf/RegionClient.proto PRE-CREATION src/main/protobuf/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 193

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>

        >

        > What is this implementing from current Delete? The delete family?

        >

        > This is returned to the client?

        >

        > How do we do a column that has no qualifier? Thats possible in hbase.

        Jimmy Xiang wrote:

        To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true.

        To delete all version of a column, you specify the family, qualifier

        What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?

        Michael Stack wrote:

        Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it).

        Jimmy Xiang wrote:

        In this case, all columns in the family for this row will be deleted, right?

        Michael Stack wrote:

        IIRC, yes. See http://hbase.apache.org/book.html#delete where I tried to doc the options

        I see. Will cover this scenario.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>

        >

        > Why is the Get polluted by multiGet stuff?

        Jimmy Xiang wrote:

        The interface is kind of similar, but very flexible. Any problem with that?

        Michael Stack wrote:

        On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok.

        Jimmy Xiang wrote:

        I see. I will remove the region from Get. It is ok not to support multiGet, right?

        Michael Stack wrote:

        We should support multiget, yes. Do you have to make a new message to do that if you remove the regionspec from Get? If so, maybe that is ok? If not, i'm fine w/ it as optional regionspec in Get. I think it would be fine to purge regionspec from the Get, Put, Delete, objects and then add messages to support multactions. Up to you. Just doc it and be consistent (smile).

        Got it.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 214

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>

        >

        > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        Jimmy Xiang wrote:

        The same reason as for Get. It is used as a default region in case all/most mutates are for a same region.

        Michael Stack wrote:

        Having 'default' region doesn't sound right, even if you are trying to be flexible.

        Jimmy Xiang wrote:

        I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several

        call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call.

        Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request?

        Michael Stack wrote:

        On "I can try to handle RowMutations in the multi call.", good. So, we won't do multiple calls to do many?

        That's right. We don't do multiple calls to do many. I will simplify the interface.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 193 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193 > > > What is this implementing from current Delete? The delete family? > > This is returned to the client? > > How do we do a column that has no qualifier? Thats possible in hbase. Jimmy Xiang wrote: To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true. To delete all version of a column, you specify the family, qualifier What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null? Michael Stack wrote: Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it). Jimmy Xiang wrote: In this case, all columns in the family for this row will be deleted, right? Michael Stack wrote: IIRC, yes. See http://hbase.apache.org/book.html#delete where I tried to doc the options I see. Will cover this scenario. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 84 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84 > > > Why is the Get polluted by multiGet stuff? Jimmy Xiang wrote: The interface is kind of similar, but very flexible. Any problem with that? Michael Stack wrote: On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok. Jimmy Xiang wrote: I see. I will remove the region from Get. It is ok not to support multiGet, right? Michael Stack wrote: We should support multiget, yes. Do you have to make a new message to do that if you remove the regionspec from Get? If so, maybe that is ok? If not, i'm fine w/ it as optional regionspec in Get. I think it would be fine to purge regionspec from the Get, Put, Delete, objects and then add messages to support multactions. Up to you. Just doc it and be consistent (smile). Got it. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 214 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214 > > > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? Jimmy Xiang wrote: The same reason as for Get. It is used as a default region in case all/most mutates are for a same region. Michael Stack wrote: Having 'default' region doesn't sound right, even if you are trying to be flexible. Jimmy Xiang wrote: I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call. Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request? Michael Stack wrote: On "I can try to handle RowMutations in the multi call.", good. So, we won't do multiple calls to do many? That's right. We don't do multiple calls to do many. I will simplify the interface. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 338

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338>

        >

        > Does this belong here? You provide it as a param when making the request.

        Jimmy Xiang wrote:

        Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right?

        Michael Stack wrote:

        Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty.

        Jimmy Xiang wrote:

        I see. I will move it out to the request so it is easier to understand.

        I think this would be good.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 233

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233>

        >

        > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first?

        Jimmy Xiang wrote:

        Yes, it is the first region.

        Michael Stack wrote:

        You know what I am going to say (smile)

        Jimmy Xiang wrote:

        Right. Shall I rename it to startRegion, or move it out, or just get rid of it and figure out the region dynamically based on the startRow?

        My guess is that this region in a Scan won't do you any good. The client figures where the Scan is at any one time and what region it should be on. There is probably no value having this in the Scan.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 35

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35>

        >

        > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them?

        Jimmy Xiang wrote:

        I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside.

        What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together.

        Michael Stack wrote:

        Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into.

        Jimmy Xiang wrote:

        Ok, I can move them back. We can get back to this when time comes.

        That might be easiest. I think yeah, they don't really belong in here and we've made some work undoing their need to be i here but lets do that in another issue....

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 87

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87>

        >

        > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response?

        Jimmy Xiang wrote:

        I was thinking to set it if the HRegion.flushCache call returned true. It is just informational.

        Michael Stack wrote:

        Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete.

        Jimmy Xiang wrote:

        I checked the code and it seems that flushCache is a synchronous call. Should we make it async? If so, I can make flushed to queued.

        Then I was wrong in my supposition. Lets do what you suggest, an actual return with a response w/ some value in it.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 66

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66>

        >

        > This is new feature on get? Or just special handling of an attribute?

        Jimmy Xiang wrote:

        This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute.

        Michael Stack wrote:

        The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get?

        Jimmy Xiang wrote:

        It is all in the server side. I can do it in the client side and remove it. That means some wasted network bandwidth. What do you think?

        I think we do what is currently there first and then after pb goes in, work on changing it. This extra flag in Get is fine (Check exists could be done as a filter if we were going to redo). Lets add it to make Get flexible as you suggest we do elsewhere in these remarks.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 71

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71>

        >

        > We don't have this in the java Result. I don't understand why this is making its way into the object.

        Jimmy Xiang wrote:

        In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name.

        In this case, I'd like the server to return the region name. This is optional. In most call, it is not used.

        Michael Stack wrote:

        So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses?

        Jimmy Xiang wrote:

        Right. Should I create a separate message for MultiResponse?

        On 'Should I create a separate message for MultiResponse?', I defer to you. You have your head in this stuff. It sounds cleaner to me having the Mutate being one message and then Mutate+regionspec be a new Message that is used trying to do Multi stuff but you are closer to what is going on here.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>

        >

        > Why is the Get polluted by multiGet stuff?

        Jimmy Xiang wrote:

        The interface is kind of similar, but very flexible. Any problem with that?

        Michael Stack wrote:

        On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok.

        Jimmy Xiang wrote:

        I see. I will remove the region from Get. It is ok not to support multiGet, right?

        We should support multiget, yes. Do you have to make a new message to do that if you remove the regionspec from Get? If so, maybe that is ok? If not, i'm fine w/ it as optional regionspec in Get. I think it would be fine to purge regionspec from the Get, Put, Delete, objects and then add messages to support multactions. Up to you. Just doc it and be consistent (smile).

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 193

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>

        >

        > What is this implementing from current Delete? The delete family?

        >

        > This is returned to the client?

        >

        > How do we do a column that has no qualifier? Thats possible in hbase.

        Jimmy Xiang wrote:

        To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true.

        To delete all version of a column, you specify the family, qualifier

        What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?

        Michael Stack wrote:

        Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it).

        Jimmy Xiang wrote:

        In this case, all columns in the family for this row will be deleted, right?

        IIRC, yes. See http://hbase.apache.org/book.html#delete where I tried to doc the options

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 214

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>

        >

        > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        Jimmy Xiang wrote:

        The same reason as for Get. It is used as a default region in case all/most mutates are for a same region.

        Michael Stack wrote:

        Having 'default' region doesn't sound right, even if you are trying to be flexible.

        Jimmy Xiang wrote:

        I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several

        call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call.

        Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request?

        On "I can try to handle RowMutations in the multi call.", good. So, we won't do multiple calls to do many?

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 338 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338 > > > Does this belong here? You provide it as a param when making the request. Jimmy Xiang wrote: Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right? Michael Stack wrote: Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty. Jimmy Xiang wrote: I see. I will move it out to the request so it is easier to understand. I think this would be good. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 233 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233 > > > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first? Jimmy Xiang wrote: Yes, it is the first region. Michael Stack wrote: You know what I am going to say (smile) Jimmy Xiang wrote: Right. Shall I rename it to startRegion, or move it out, or just get rid of it and figure out the region dynamically based on the startRow? My guess is that this region in a Scan won't do you any good. The client figures where the Scan is at any one time and what region it should be on. There is probably no value having this in the Scan. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 35 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35 > > > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them? Jimmy Xiang wrote: I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside. What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together. Michael Stack wrote: Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into. Jimmy Xiang wrote: Ok, I can move them back. We can get back to this when time comes. That might be easiest. I think yeah, they don't really belong in here and we've made some work undoing their need to be i here but lets do that in another issue.... On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 87 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87 > > > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response? Jimmy Xiang wrote: I was thinking to set it if the HRegion.flushCache call returned true. It is just informational. Michael Stack wrote: Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete. Jimmy Xiang wrote: I checked the code and it seems that flushCache is a synchronous call. Should we make it async? If so, I can make flushed to queued. Then I was wrong in my supposition. Lets do what you suggest, an actual return with a response w/ some value in it. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 66 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66 > > > This is new feature on get? Or just special handling of an attribute? Jimmy Xiang wrote: This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute. Michael Stack wrote: The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get? Jimmy Xiang wrote: It is all in the server side. I can do it in the client side and remove it. That means some wasted network bandwidth. What do you think? I think we do what is currently there first and then after pb goes in, work on changing it. This extra flag in Get is fine (Check exists could be done as a filter if we were going to redo). Lets add it to make Get flexible as you suggest we do elsewhere in these remarks. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 71 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71 > > > We don't have this in the java Result. I don't understand why this is making its way into the object. Jimmy Xiang wrote: In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name. In this case, I'd like the server to return the region name. This is optional. In most call, it is not used. Michael Stack wrote: So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses? Jimmy Xiang wrote: Right. Should I create a separate message for MultiResponse? On 'Should I create a separate message for MultiResponse?', I defer to you. You have your head in this stuff. It sounds cleaner to me having the Mutate being one message and then Mutate+regionspec be a new Message that is used trying to do Multi stuff but you are closer to what is going on here. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 84 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84 > > > Why is the Get polluted by multiGet stuff? Jimmy Xiang wrote: The interface is kind of similar, but very flexible. Any problem with that? Michael Stack wrote: On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok. Jimmy Xiang wrote: I see. I will remove the region from Get. It is ok not to support multiGet, right? We should support multiget, yes. Do you have to make a new message to do that if you remove the regionspec from Get? If so, maybe that is ok? If not, i'm fine w/ it as optional regionspec in Get. I think it would be fine to purge regionspec from the Get, Put, Delete, objects and then add messages to support multactions. Up to you. Just doc it and be consistent (smile). On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 193 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193 > > > What is this implementing from current Delete? The delete family? > > This is returned to the client? > > How do we do a column that has no qualifier? Thats possible in hbase. Jimmy Xiang wrote: To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true. To delete all version of a column, you specify the family, qualifier What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null? Michael Stack wrote: Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it). Jimmy Xiang wrote: In this case, all columns in the family for this row will be deleted, right? IIRC, yes. See http://hbase.apache.org/book.html#delete where I tried to doc the options On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 214 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214 > > > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? Jimmy Xiang wrote: The same reason as for Get. It is used as a default region in case all/most mutates are for a same region. Michael Stack wrote: Having 'default' region doesn't sound right, even if you are trying to be flexible. Jimmy Xiang wrote: I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call. Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request? On "I can try to handle RowMutations in the multi call.", good. So, we won't do multiple calls to do many? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 87

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87>

        >

        > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response?

        Jimmy Xiang wrote:

        I was thinking to set it if the HRegion.flushCache call returned true. It is just informational.

        Michael Stack wrote:

        Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete.

        I checked the code and it seems that flushCache is a synchronous call. Should we make it async? If so, I can make flushed to queued.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 70

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70>

        >

        > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think?

        Jimmy Xiang wrote:

        I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise.

        Michael Stack wrote:

        I'd be fine w/ the doc being sparse but there are a few cases where doc is necessary; e.g. the one I cite above.

        Sure, I will add more doc where confusion could happen.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 35

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35>

        >

        > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them?

        Jimmy Xiang wrote:

        I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside.

        What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together.

        Michael Stack wrote:

        Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into.

        Ok, I can move them back. We can get back to this when time comes.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 77

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77>

        >

        > what is processed?

        Jimmy Xiang wrote:

        Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case.

        Michael Stack wrote:

        Needs comment

        Will add comment

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 159

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159>

        >

        > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves?

        >

        > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate?

        Jimmy Xiang wrote:

        Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods.

        Michael Stack wrote:

        'But for RPC, I think it is better to have fewer and flexible methods.' Fair enough. Would like to make it just flexible enough and no more (smile). Needs documentation too else fellas like me will come in and get wrong impression on whats going on.

        Sure. I will make it simple for now. We can enhance if requirements come up.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 87 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87 > > > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response? Jimmy Xiang wrote: I was thinking to set it if the HRegion.flushCache call returned true. It is just informational. Michael Stack wrote: Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete. I checked the code and it seems that flushCache is a synchronous call. Should we make it async? If so, I can make flushed to queued. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 70 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70 > > > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think? Jimmy Xiang wrote: I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise. Michael Stack wrote: I'd be fine w/ the doc being sparse but there are a few cases where doc is necessary; e.g. the one I cite above. Sure, I will add more doc where confusion could happen. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 35 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35 > > > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them? Jimmy Xiang wrote: I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside. What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together. Michael Stack wrote: Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into. Ok, I can move them back. We can get back to this when time comes. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 77 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77 > > > what is processed? Jimmy Xiang wrote: Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case. Michael Stack wrote: Needs comment Will add comment On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 159 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159 > > > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves? > > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate? Jimmy Xiang wrote: Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods. Michael Stack wrote: 'But for RPC, I think it is better to have fewer and flexible methods.' Fair enough. Would like to make it just flexible enough and no more (smile). Needs documentation too else fellas like me will come in and get wrong impression on whats going on. Sure. I will make it simple for now. We can enhance if requirements come up. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 338

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338>

        >

        > Does this belong here? You provide it as a param when making the request.

        Jimmy Xiang wrote:

        Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right?

        Michael Stack wrote:

        Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty.

        I see. I will move it out to the request so it is easier to understand.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 233

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233>

        >

        > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first?

        Jimmy Xiang wrote:

        Yes, it is the first region.

        Michael Stack wrote:

        You know what I am going to say (smile)

        Right. Shall I rename it to startRegion, or move it out, or just get rid of it and figure out the region dynamically based on the startRow?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 193

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>

        >

        > What is this implementing from current Delete? The delete family?

        >

        > This is returned to the client?

        >

        > How do we do a column that has no qualifier? Thats possible in hbase.

        Jimmy Xiang wrote:

        To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true.

        To delete all version of a column, you specify the family, qualifier

        What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?

        Michael Stack wrote:

        Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it).

        In this case, all columns in the family for this row will be deleted, right?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 91

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91>

        >

        > I thought we could set this into the Get above? Why have it here as separate param?

        Jimmy Xiang wrote:

        This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where.

        Michael Stack wrote:

        Could we just have it out here as a param on the get and not in the Get object? Will that work (I think I understand why Result needs it internal to cut down on objects we need to support multiresponses)

        Sure, I can do that for get.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>

        >

        > Why is the Get polluted by multiGet stuff?

        Jimmy Xiang wrote:

        The interface is kind of similar, but very flexible. Any problem with that?

        Michael Stack wrote:

        On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok.

        I see. I will remove the region from Get. It is ok not to support multiGet, right?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 82

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82>

        >

        > Why we need to add a region to the Get even optionally?

        Jimmy Xiang wrote:

        That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level.

        Michael Stack wrote:

        I see specifying the region name in this GetRequest but not sure about why we'd have region in the Get object itself if we are doing it here on the invocation. Seems like the two contend?

        I will remove it from Get object to avoid confusion for now.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 71

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71>

        >

        > We don't have this in the java Result. I don't understand why this is making its way into the object.

        Jimmy Xiang wrote:

        In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name.

        In this case, I'd like the server to return the region name. This is optional. In most call, it is not used.

        Michael Stack wrote:

        So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses?

        Right. Should I create a separate message for MultiResponse?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 66

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66>

        >

        > This is new feature on get? Or just special handling of an attribute?

        Jimmy Xiang wrote:

        This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute.

        Michael Stack wrote:

        The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get?

        It is all in the server side. I can do it in the client side and remove it. That means some wasted network bandwidth. What do you think?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 214

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>

        >

        > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        Jimmy Xiang wrote:

        The same reason as for Get. It is used as a default region in case all/most mutates are for a same region.

        Michael Stack wrote:

        Having 'default' region doesn't sound right, even if you are trying to be flexible.

        I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several
        call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call.

        Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request?

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 338 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338 > > > Does this belong here? You provide it as a param when making the request. Jimmy Xiang wrote: Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right? Michael Stack wrote: Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty. I see. I will move it out to the request so it is easier to understand. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 233 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233 > > > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first? Jimmy Xiang wrote: Yes, it is the first region. Michael Stack wrote: You know what I am going to say (smile) Right. Shall I rename it to startRegion, or move it out, or just get rid of it and figure out the region dynamically based on the startRow? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 193 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193 > > > What is this implementing from current Delete? The delete family? > > This is returned to the client? > > How do we do a column that has no qualifier? Thats possible in hbase. Jimmy Xiang wrote: To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true. To delete all version of a column, you specify the family, qualifier What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null? Michael Stack wrote: Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it). In this case, all columns in the family for this row will be deleted, right? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 91 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91 > > > I thought we could set this into the Get above? Why have it here as separate param? Jimmy Xiang wrote: This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where. Michael Stack wrote: Could we just have it out here as a param on the get and not in the Get object? Will that work (I think I understand why Result needs it internal to cut down on objects we need to support multiresponses) Sure, I can do that for get. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 84 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84 > > > Why is the Get polluted by multiGet stuff? Jimmy Xiang wrote: The interface is kind of similar, but very flexible. Any problem with that? Michael Stack wrote: On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok. I see. I will remove the region from Get. It is ok not to support multiGet, right? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 82 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82 > > > Why we need to add a region to the Get even optionally? Jimmy Xiang wrote: That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level. Michael Stack wrote: I see specifying the region name in this GetRequest but not sure about why we'd have region in the Get object itself if we are doing it here on the invocation. Seems like the two contend? I will remove it from Get object to avoid confusion for now. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 71 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71 > > > We don't have this in the java Result. I don't understand why this is making its way into the object. Jimmy Xiang wrote: In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name. In this case, I'd like the server to return the region name. This is optional. In most call, it is not used. Michael Stack wrote: So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses? Right. Should I create a separate message for MultiResponse? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 66 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66 > > > This is new feature on get? Or just special handling of an attribute? Jimmy Xiang wrote: This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute. Michael Stack wrote: The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get? It is all in the server side. I can do it in the client side and remove it. That means some wasted network bandwidth. What do you think? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 214 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214 > > > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? Jimmy Xiang wrote: The same reason as for Get. It is used as a default region in case all/most mutates are for a same region. Michael Stack wrote: Having 'default' region doesn't sound right, even if you are trying to be flexible. I see. For get, we put the region in the request level. I prefer to do the same for mutate. But if we put the region in request for mutate, we have to have a several call to handle RowMutations in the same call. I can try to handle RowMutations in the multi call. Another thing is that, if we put the region in the request level, do we need to support multiple Get per get request? How about multiple Mutate per mutate request? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 21

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line21>

        >

        > What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto.

        Jimmy Xiang wrote:

        There is already an exiting folder called protobuf (rest). Let me change the dir holds the .proto files under src/main to protobuf.

        good

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 35

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35>

        >

        > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them?

        Jimmy Xiang wrote:

        I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside.

        What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together.

        Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 70

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70>

        >

        > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think?

        Jimmy Xiang wrote:

        I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise.

        I'd be fine w/ the doc being sparse but there are a few cases where doc is necessary; e.g. the one I cite above.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 87

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87>

        >

        > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response?

        Jimmy Xiang wrote:

        I was thinking to set it if the HRegion.flushCache call returned true. It is just informational.

        Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 52

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line52>

        >

        > This is new? Being able to do this? How will it be used?

        Jimmy Xiang wrote:

        Some call expects a region name, some call expects an encoded region name. With a specifier, we can handle both.

        Encoded region name works only if the region is on-line. If we can't find the region based on the specifier, a region not found exception will be thrown.

        So we can simplify the request a little bit since we don't have to ask for region name, and encoded region name, and check only if one is specified.

        I will add a comment for it.

        ok

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 66

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66>

        >

        > This is new feature on get? Or just special handling of an attribute?

        Jimmy Xiang wrote:

        This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute.

        The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 71

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71>

        >

        > We don't have this in the java Result. I don't understand why this is making its way into the object.

        Jimmy Xiang wrote:

        In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name.

        In this case, I'd like the server to return the region name. This is optional. In most call, it is not used.

        So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 77

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77>

        >

        > what is processed?

        Jimmy Xiang wrote:

        Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case.

        Needs comment

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 82

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82>

        >

        > Why we need to add a region to the Get even optionally?

        Jimmy Xiang wrote:

        That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level.

        I see specifying the region name in this GetRequest but not sure about why we'd have region in the Get object itself if we are doing it here on the invocation. Seems like the two contend?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>

        >

        > Why is the Get polluted by multiGet stuff?

        Jimmy Xiang wrote:

        The interface is kind of similar, but very flexible. Any problem with that?

        On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 91

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91>

        >

        > I thought we could set this into the Get above? Why have it here as separate param?

        Jimmy Xiang wrote:

        This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where.

        Could we just have it out here as a param on the get and not in the Get object? Will that work (I think I understand why Result needs it internal to cut down on objects we need to support multiresponses)

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 159

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159>

        >

        > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves?

        >

        > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate?

        Jimmy Xiang wrote:

        Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods.

        'But for RPC, I think it is better to have fewer and flexible methods.' Fair enough. Would like to make it just flexible enough and no more (smile). Needs documentation too else fellas like me will come in and get wrong impression on whats going on.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 193

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>

        >

        > What is this implementing from current Delete? The delete family?

        >

        > This is returned to the client?

        >

        > How do we do a column that has no qualifier? Thats possible in hbase.

        Jimmy Xiang wrote:

        To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true.

        To delete all version of a column, you specify the family, qualifier

        What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?

        Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it).

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 214

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>

        >

        > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        Jimmy Xiang wrote:

        The same reason as for Get. It is used as a default region in case all/most mutates are for a same region.

        Having 'default' region doesn't sound right, even if you are trying to be flexible.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 233

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233>

        >

        > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first?

        Jimmy Xiang wrote:

        Yes, it is the first region.

        You know what I am going to say (smile)

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 338

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338>

        >

        > Does this belong here? You provide it as a param when making the request.

        Jimmy Xiang wrote:

        Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right?

        Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 396

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line396>

        >

        > Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96.

        Jimmy Xiang wrote:

        It is still there. It is collapsed into the Get call.

        ok. good.

        • Michael

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 21 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line21 > > > What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto. Jimmy Xiang wrote: There is already an exiting folder called protobuf (rest). Let me change the dir holds the .proto files under src/main to protobuf. good On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 35 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35 > > > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them? Jimmy Xiang wrote: I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside. What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together. Hmm. Moving out these flags changes the current 'model' but in a direction we should be headed in. The split/offline stuff were stuffed into HRI in the first place just because this was an easy way to pass these transient states around in hbase; they also are less important now in HRI though still depended on when we scan meta IIRC. Its probably better to evolve toward an HRI that is immutable once made. So I'd be down w/ moving them out but its up to you. It might be easier on you achieving parity w/ first commit of pb work if the pb classes are like the internals they will be feeding into. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 70 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70 > > > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think? Jimmy Xiang wrote: I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise. I'd be fine w/ the doc being sparse but there are a few cases where doc is necessary; e.g. the one I cite above. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 87 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87 > > > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response? Jimmy Xiang wrote: I was thinking to set it if the HRegion.flushCache call returned true. It is just informational. Does that imply a synchronous call? I thought flushCache just queued the flush then returned to the client w/o actually waiting on flush to complete. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 52 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line52 > > > This is new? Being able to do this? How will it be used? Jimmy Xiang wrote: Some call expects a region name, some call expects an encoded region name. With a specifier, we can handle both. Encoded region name works only if the region is on-line. If we can't find the region based on the specifier, a region not found exception will be thrown. So we can simplify the request a little bit since we don't have to ask for region name, and encoded region name, and check only if one is specified. I will add a comment for it. ok On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 66 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66 > > > This is new feature on get? Or just special handling of an attribute? Jimmy Xiang wrote: This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute. The current implementation actually does the fetch and in the client checks it null or not IIRC? Or is it all serverside? So you add this to the Get so you don't have to do the exists call? You can implement it w/ the addition to Get? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 71 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71 > > > We don't have this in the java Result. I don't understand why this is making its way into the object. Jimmy Xiang wrote: In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name. In this case, I'd like the server to return the region name. This is optional. In most call, it is not used. So you are doing this so you have to create less objects? So you can avoid MultiResponse with its regionname to responses? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 77 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77 > > > what is processed? Jimmy Xiang wrote: Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case. Needs comment On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 82 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82 > > > Why we need to add a region to the Get even optionally? Jimmy Xiang wrote: That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level. I see specifying the region name in this GetRequest but not sure about why we'd have region in the Get object itself if we are doing it here on the invocation. Seems like the two contend? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 84 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84 > > > Why is the Get polluted by multiGet stuff? Jimmy Xiang wrote: The interface is kind of similar, but very flexible. Any problem with that? On 'Any problem with that?', no. Just trying to avoid redundancy that makes the Interface imprecise and therefore hard to grok. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 91 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91 > > > I thought we could set this into the Get above? Why have it here as separate param? Jimmy Xiang wrote: This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where. Could we just have it out here as a param on the get and not in the Get object? Will that work (I think I understand why Result needs it internal to cut down on objects we need to support multiresponses) On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 159 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159 > > > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves? > > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate? Jimmy Xiang wrote: Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods. 'But for RPC, I think it is better to have fewer and flexible methods.' Fair enough. Would like to make it just flexible enough and no more (smile). Needs documentation too else fellas like me will come in and get wrong impression on whats going on. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 193 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193 > > > What is this implementing from current Delete? The delete family? > > This is returned to the client? > > How do we do a column that has no qualifier? Thats possible in hbase. Jimmy Xiang wrote: To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true. To delete all version of a column, you specify the family, qualifier What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null? Yeah. You can put a value at row, family, null qualifier, ts. You can also delete it (might not be a good idea but you can do it). On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 214 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214 > > > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? Jimmy Xiang wrote: The same reason as for Get. It is used as a default region in case all/most mutates are for a same region. Having 'default' region doesn't sound right, even if you are trying to be flexible. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 233 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233 > > > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first? Jimmy Xiang wrote: Yes, it is the first region. You know what I am going to say (smile) On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 338 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338 > > > Does this belong here? You provide it as a param when making the request. Jimmy Xiang wrote: Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right? Yes. I got into habit of complaining about the region specs. They make sense when they are params. Its regions internal to Get, etc., that causes me difficulty. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 396 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line396 > > > Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96. Jimmy Xiang wrote: It is still there. It is collapsed into the Get call. ok. good. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > Excellent.

        >

        > It looks like we can commit this w/o breaking whats currently there?

        That's right. It won't break anything. It is just proto files in this patch. Thanks for reviewing.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 21

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line21>

        >

        > What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto.

        There is already an exiting folder called protobuf (rest). Let me change the dir holds the .proto files under src/main to protobuf.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 35

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35>

        >

        > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them?

        I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside.

        What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 70

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70>

        >

        > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think?

        I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 87

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87>

        >

        > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response?

        I was thinking to set it if the HRegion.flushCache call returned true. It is just informational.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 112

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line112>

        >

        > WALKey maps to HLogKey? Maybe add a comment to this effect?

        Sure. Will add a comment.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 141

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line141>

        >

        > Good. I like this method name better. Should be a comment which points back to the old name? Or what you think?

        Ok. Will do.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionAdmin.proto, line 151

        > <https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line151>

        >

        > Yeah, this proto is missing commentary. I mean, the return here should be explained?

        Ok, will add a comment too.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 52

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line52>

        >

        > This is new? Being able to do this? How will it be used?

        Some call expects a region name, some call expects an encoded region name. With a specifier, we can handle both.
        Encoded region name works only if the region is on-line. If we can't find the region based on the specifier, a region not found exception will be thrown.

        So we can simplify the request a little bit since we don't have to ask for region name, and encoded region name, and check only if one is specified.

        I will add a comment for it.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 66

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66>

        >

        > This is new feature on get? Or just special handling of an attribute?

        This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 71

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71>

        >

        > We don't have this in the java Result. I don't understand why this is making its way into the object.

        In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name.
        In this case, I'd like the server to return the region name. This is optional. In most call, it is not used.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 74

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line74>

        >

        > ditto

        It is used when the caller wants the existence only, not the result itself.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 77

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77>

        >

        > what is processed?

        Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 82

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82>

        >

        > Why we need to add a region to the Get even optionally?

        That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 396

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line396>

        >

        > Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96.

        It is still there. It is collapsed into the Get call.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 156

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line156>

        >

        > A type rather than have the mutation type specified as a subclass?

        >

        > A mutate is a delete or put?

        It could be a delete or a put.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84>

        >

        > Why is the Get polluted by multiGet stuff?

        The interface is kind of similar, but very flexible. Any problem with that?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 91

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91>

        >

        > I thought we could set this into the Get above? Why have it here as separate param?

        This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 147

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line147>

        >

        > This is a new feature?

        It is for checkAndPut and checkAndDelete.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 159

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159>

        >

        > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves?

        >

        > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate?

        Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 193

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193>

        >

        > What is this implementing from current Delete? The delete family?

        >

        > This is returned to the client?

        >

        > How do we do a column that has no qualifier? Thats possible in hbase.

        To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true.
        To delete all version of a column, you specify the family, qualifier

        What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null?

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 214

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214>

        >

        > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        The same reason as for Get. It is used as a default region in case all/most mutates are for a same region.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 233

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233>

        >

        > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first?

        Yes, it is the first region.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 276

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line276>

        >

        > Is this supposed to be row specifier?

        It is the region where the row is on. The row is in line 277.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 286

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line286>

        >

        > ditto

        same as LockRowRequest.

        On 2012-03-26 23:21:58, Michael Stack wrote:

        > src/main/proto/RegionClient.proto, line 338

        > <https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338>

        >

        > Does this belong here? You provide it as a param when making the request.

        Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right?

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 23:21:58, Michael Stack wrote: > Excellent. > > It looks like we can commit this w/o breaking whats currently there? That's right. It won't break anything. It is just proto files in this patch. Thanks for reviewing. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 21 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line21 > > > What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto. There is already an exiting folder called protobuf (rest). Let me change the dir holds the .proto files under src/main to protobuf. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 35 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line35 > > > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them? I used to put these transient parameters in the protobuff RegionInfo as well. However Todd thought it's better to put them outside. What'd do you think? To me, it is fine either way. However, if we are going to replace HRI with the protobuff later on, it may be better to put them together. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 70 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line70 > > > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think? I though about this. I added documentation to RegionClient.proto. For methods in RegionAdmin, the method names seem to be very clear. I will take a look again and document where confusing/misunderstand could arise. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 87 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line87 > > > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response? I was thinking to set it if the HRegion.flushCache call returned true. It is just informational. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 112 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line112 > > > WALKey maps to HLogKey? Maybe add a comment to this effect? Sure. Will add a comment. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 141 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line141 > > > Good. I like this method name better. Should be a comment which points back to the old name? Or what you think? Ok. Will do. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionAdmin.proto, line 151 > < https://reviews.apache.org/r/4054/diff/5/?file=95915#file95915line151 > > > Yeah, this proto is missing commentary. I mean, the return here should be explained? Ok, will add a comment too. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 52 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line52 > > > This is new? Being able to do this? How will it be used? Some call expects a region name, some call expects an encoded region name. With a specifier, we can handle both. Encoded region name works only if the region is on-line. If we can't find the region based on the specifier, a region not found exception will be thrown. So we can simplify the request a little bit since we don't have to ask for region name, and encoded region name, and check only if one is specified. I will add a comment for it. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 66 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line66 > > > This is new feature on get? Or just special handling of an attribute? This is for the exist() call. In this case, the caller doesn't care about the result. They just want to know if the row is there. It is not special handling of an attribute. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 71 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line71 > > > We don't have this in the java Result. I don't understand why this is making its way into the object. In the multi call, they mix many requests together. However, they don't retrieve the response in order. They retrieve the response based on the region name. In this case, I'd like the server to return the region name. This is optional. In most call, it is not used. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 74 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line74 > > > ditto It is used when the caller wants the existence only, not the result itself. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 77 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line77 > > > what is processed? Sometimes, for some action, for example delete, they don't care about the result. They just want to know if the action is processed or not. This is for this kind use case. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 82 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line82 > > > Why we need to add a region to the Get even optionally? That's to support multiple Get requests in one call. Each Get request can have its own region. If all Gets are for one region, the caller can specify the region in the request level. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 396 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line396 > > > Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96. It is still there. It is collapsed into the Get call. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 156 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line156 > > > A type rather than have the mutation type specified as a subclass? > > A mutate is a delete or put? It could be a delete or a put. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 84 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line84 > > > Why is the Get polluted by multiGet stuff? The interface is kind of similar, but very flexible. Any problem with that? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 91 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line91 > > > I thought we could set this into the Get above? Why have it here as separate param? This is used as a default region. If you have multiple Gets on the same region, you don't have to specify it every where. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 147 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line147 > > > This is a new feature? It is for checkAndPut and checkAndDelete. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 159 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line159 > > > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves? > > Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate? Similar to Gets, I'd like to make the interface flexible. Sounds you prefer to have simple interface instead, right? That means we may have to have too many methods. For java interface, it is ok and preferred to have many methods. But for RPC, I think it is better to have fewer and flexible methods. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 193 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line193 > > > What is this implementing from current Delete? The delete family? > > This is returned to the client? > > How do we do a column that has no qualifier? Thats possible in hbase. To delete a family, you don't specify the qualifier. To delete a only one version of a column, you sepcify the family, qualifer, timestamp, and set oneVersion to true. To delete all version of a column, you specify the family, qualifier What do you mean a column that has no qualifier? Are you referring to Delete.deleteColumns(family, qualifier, timestamp), and qualifer = null? On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 214 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line214 > > > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? The same reason as for Get. It is used as a default region in case all/most mutates are for a same region. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 233 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line233 > > > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first? Yes, it is the first region. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 276 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line276 > > > Is this supposed to be row specifier? It is the region where the row is on. The row is in line 277. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 286 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line286 > > > ditto same as LockRowRequest. On 2012-03-26 23:21:58, Michael Stack wrote: > src/main/proto/RegionClient.proto, line 338 > < https://reviews.apache.org/r/4054/diff/5/?file=95916#file95916line338 > > > Does this belong here? You provide it as a param when making the request. Yes, this is for Region protocol, not client API. You need to specify for which region this action will be executed on, right? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6365
        -----------------------------------------------------------

        Excellent.

        It looks like we can commit this w/o breaking whats currently there?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13813>

        What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto.

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13835>

        What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13836>

        Good. Its kinda dumb the way our HRegionInterface is now where it has an override, one that takes single family and another that takes an array. Thanks for collapsing.

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13837>

        Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13838>

        Again, thanks for doing the work collapsing the overrides that are up in HRegionInterface.

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13839>

        Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13842>

        WALKey maps to HLogKey? Maybe add a comment to this effect?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13846>

        Good. I like this method name better. Should be a comment which points back to the old name? Or what you think?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13847>

        Yeah, this proto is missing commentary. I mean, the return here should be explained?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13850>

        This will for sure grow w/ time.

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13855>

        Nice. So you are splitting HRegionInterface into admin and client? Good.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13862>

        This is new? Being able to do this? How will it be used?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13863>

        This is new feature on get? Or just special handling of an attribute?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13864>

        We don't have this in the java Result. I don't understand why this is making its way into the object.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13865>

        ditto

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13866>

        what is processed?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13867>

        Why we need to add a region to the Get even optionally?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13868>

        Why is the Get polluted by multiGet stuff?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13869>

        I thought we could set this into the Get above? Why have it here as separate param?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13870>

        Good stuff

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13872>

        This is a new feature?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13881>

        A type rather than have the mutation type specified as a subclass?

        A mutate is a delete or put?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13882>

        Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves?

        Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13885>

        What is this implementing from current Delete? The delete family?

        This is returned to the client?

        How do we do a column that has no qualifier? Thats possible in hbase.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13890>

        Yeah, why have regionspecified in the mutate if you are going to provide it as a param too?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13898>

        Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13899>

        Is this supposed to be row specifier?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13900>

        ditto

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13902>

        Does this belong here?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13905>

        Does this belong here? You provide it as a param when making the request.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13906>

        ditto

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13908>

        Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment13909>

        Good

        • Michael

        On 2012-03-26 20:14:22, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6365 ----------------------------------------------------------- Excellent. It looks like we can commit this w/o breaking whats currently there? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13813 > What did we figure on the package name? Shouldn't it agree w/ the dir that holds the .proto files at src/main? Currently one is protobuf and the other is proto. src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13835 > What are these two booleans broken out? Aren't they in they attributes of HRI already? Why repeat them? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13836 > Good. Its kinda dumb the way our HRegionInterface is now where it has an override, one that takes single family and another that takes an array. Thanks for collapsing. src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13837 > Should we be repeating the API documentation that is up in the HRegionInterface that this .proto replaces here? If its not here, where will it be? Not all of the javadoc should make it over – the stuff that says nothing shouldn't but some is of worth. What you think? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13838 > Again, thanks for doing the work collapsing the overrides that are up in HRegionInterface. src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13839 > Isn't the response currently a void? And isnt' flush async (IIRC). If so, under what circumstance would we be able to fill out this response? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13842 > WALKey maps to HLogKey? Maybe add a comment to this effect? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13846 > Good. I like this method name better. Should be a comment which points back to the old name? Or what you think? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13847 > Yeah, this proto is missing commentary. I mean, the return here should be explained? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13850 > This will for sure grow w/ time. src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13855 > Nice. So you are splitting HRegionInterface into admin and client? Good. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13862 > This is new? Being able to do this? How will it be used? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13863 > This is new feature on get? Or just special handling of an attribute? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13864 > We don't have this in the java Result. I don't understand why this is making its way into the object. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13865 > ditto src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13866 > what is processed? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13867 > Why we need to add a region to the Get even optionally? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13868 > Why is the Get polluted by multiGet stuff? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13869 > I thought we could set this into the Get above? Why have it here as separate param? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13870 > Good stuff src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13872 > This is a new feature? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13881 > A type rather than have the mutation type specified as a subclass? A mutate is a delete or put? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13882 > Again, does it make sense having this in here? I mean regions come and go – e.g. split – so you could have reference to non-existent region. This stuff of tying a mutation to a particular region should be done external to the mutations themselves? Is this trying to do multiaction? Maybe that should be a message apart from mutate? The new message would have region name and the mutate? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13885 > What is this implementing from current Delete? The delete family? This is returned to the client? How do we do a column that has no qualifier? Thats possible in hbase. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13890 > Yeah, why have regionspecified in the mutate if you are going to provide it as a param too? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13898 > Why this when regions can change. Also, a Scan can traverse many regions so what is the regionspecifier referring to? The first? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13899 > Is this supposed to be row specifier? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13900 > ditto src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13902 > Does this belong here? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13905 > Does this belong here? You provide it as a param when making the request. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13906 > ditto src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13908 > Is this everything? I don't see closestrowbefore? We'll probably still need this in 0.96. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment13909 > Good Michael On 2012-03-26 20:14:22, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/
        -----------------------------------------------------------

        (Updated 2012-03-26 20:14:22.677676)

        Review request for hbase.

        Changes
        -------

        Addressed Benoit's comments.

        Summary
        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.
        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs (updated)


        pom.xml 10b13ef
        src/main/proto/RegionAdmin.proto PRE-CREATION
        src/main/proto/RegionClient.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-26 20:14:22.677676) Review request for hbase. Changes ------- Addressed Benoit's comments. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs (updated) pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-26 19:15:13, Benoit Sigoure wrote:

        > pom.xml, line 780

        > <https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line780>

        >

        > Just do "if which cygpath 2>/dev/null" instead of doing it on 2 lines and testing $?

        Sure, will fix.

        On 2012-03-26 19:15:13, Benoit Sigoure wrote:

        > pom.xml, line 787

        > <https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line787>

        >

        > Just do "for PROTO_FILE in $UNIX_PROTO_DIR/*.proto", I don't think the "ls" is really necessary here.

        You are right. It works without "ls". Fixed.

        On 2012-03-26 19:15:13, Benoit Sigoure wrote:

        > pom.xml, line 334

        > <https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line334>

        >

        > Can you please kill all the trailing whitespaces while you're at it?

        Sure, I will do that.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6354
        -----------------------------------------------------------

        On 2012-03-23 19:29:52, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-23 19:29:52)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-26 19:15:13, Benoit Sigoure wrote: > pom.xml, line 780 > < https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line780 > > > Just do "if which cygpath 2>/dev/null" instead of doing it on 2 lines and testing $? Sure, will fix. On 2012-03-26 19:15:13, Benoit Sigoure wrote: > pom.xml, line 787 > < https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line787 > > > Just do "for PROTO_FILE in $UNIX_PROTO_DIR/*.proto", I don't think the "ls" is really necessary here. You are right. It works without "ls". Fixed. On 2012-03-26 19:15:13, Benoit Sigoure wrote: > pom.xml, line 334 > < https://reviews.apache.org/r/4054/diff/1-4/?file=86002#file86002line334 > > > Can you please kill all the trailing whitespaces while you're at it? Sure, I will do that. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6354 ----------------------------------------------------------- On 2012-03-23 19:29:52, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-23 19:29:52) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6356
        -----------------------------------------------------------

        Oh my, it's just that review board sucks, it's still showing the file as being added when comparing r1 and r4, even though the file is no longer in r4.

        • Benoit

        On 2012-03-23 19:29:52, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-23 19:29:52)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6356 ----------------------------------------------------------- Oh my, it's just that review board sucks, it's still showing the file as being added when comparing r1 and r4, even though the file is no longer in r4. Benoit On 2012-03-23 19:29:52, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-23 19:29:52) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6354
        -----------------------------------------------------------

        I'm looking at differences between r1 and r4 and I don't see anything new in HRegionProtocol.proto ..?

        pom.xml
        <https://reviews.apache.org/r/4054/#comment13781>

        Can you please kill all the trailing whitespaces while you're at it?

        pom.xml
        <https://reviews.apache.org/r/4054/#comment13779>

        Just do "if which cygpath 2>/dev/null" instead of doing it on 2 lines and testing $?

        pom.xml
        <https://reviews.apache.org/r/4054/#comment13782>

        Just do "for PROTO_FILE in $UNIX_PROTO_DIR/*.proto", I don't think the "ls" is really necessary here.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment13783>

        I'm not seeing that you added it.

        • Benoit

        On 2012-03-23 19:29:52, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-23 19:29:52)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6354 ----------------------------------------------------------- I'm looking at differences between r1 and r4 and I don't see anything new in HRegionProtocol.proto ..? pom.xml < https://reviews.apache.org/r/4054/#comment13781 > Can you please kill all the trailing whitespaces while you're at it? pom.xml < https://reviews.apache.org/r/4054/#comment13779 > Just do "if which cygpath 2>/dev/null" instead of doing it on 2 lines and testing $? pom.xml < https://reviews.apache.org/r/4054/#comment13782 > Just do "for PROTO_FILE in $UNIX_PROTO_DIR/*.proto", I don't think the "ls" is really necessary here. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment13783 > I'm not seeing that you added it. Benoit On 2012-03-23 19:29:52, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-23 19:29:52) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        Ted Yu added a comment -

        @Stack, @Benoit:
        Please take a look.

        Show
        Ted Yu added a comment - @Stack, @Benoit: Please take a look.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/
        -----------------------------------------------------------

        (Updated 2012-03-23 19:29:52.180425)

        Review request for hbase.

        Changes
        -------

        Addressed Ted's comments.

        Summary
        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.
        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs (updated)


        pom.xml 10b13ef
        src/main/proto/RegionAdmin.proto PRE-CREATION
        src/main/proto/RegionClient.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-23 19:29:52.180425) Review request for hbase. Changes ------- Addressed Ted's comments. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs (updated) pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-23 18:30:08, Ted Yu wrote:

        > src/main/proto/RegionAdmin.proto, line 115

        > <https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line115>

        >

        > Introducing message UUID would be better.

        Fixed.

        • Jimmy

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6299
        -----------------------------------------------------------

        On 2012-03-22 20:20:39, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-22 20:20:39)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-23 18:30:08, Ted Yu wrote: > src/main/proto/RegionAdmin.proto, line 115 > < https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line115 > > > Introducing message UUID would be better. Fixed. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6299 ----------------------------------------------------------- On 2012-03-22 20:20:39, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:20:39) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6299
        -----------------------------------------------------------

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13666>

        Introducing message UUID would be better.

        • Ted

        On 2012-03-22 20:20:39, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-22 20:20:39)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6299 ----------------------------------------------------------- src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13666 > Introducing message UUID would be better. Ted On 2012-03-22 20:20:39, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:20:39) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > Looks good overall.

        > Minor comments below.

        Thanks for reviewing.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionAdmin.proto, line 115

        > <https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line115>

        >

        > 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?

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 147

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line147>

        >

        > Remove 'is'

        Removed

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 153

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line153>

        >

        > white space.

        Removed.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionAdmin.proto, line 171

        > <https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line171>

        >

        > 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.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 139

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line139>

        >

        > Increment doesn't extend Mutation:

        >

        > public class Increment implements Row {

        >

        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.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 171

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line171>

        >

        > Should this line be removed ?

        Increment is treated as a Mutate, so it is needed.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 208

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line208>

        >

        > 'it's appeared' -> 'it appears'

        Fixed.

        On 2012-03-23 17:54:17, Ted Yu wrote:

        > src/main/proto/RegionClient.proto, line 335

        > <https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line335>

        >

        > I don't see region member in Exec.java

        > 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:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-22 20:20:39)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-23 17:54:17, Ted Yu wrote: > Looks good overall. > Minor comments below. Thanks for reviewing. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionAdmin.proto, line 115 > < https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line115 > > > 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? On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 147 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line147 > > > Remove 'is' Removed On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 153 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line153 > > > white space. Removed. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionAdmin.proto, line 171 > < https://reviews.apache.org/r/4054/diff/3/?file=94679#file94679line171 > > > 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. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 139 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line139 > > > Increment doesn't extend Mutation: > > public class Increment implements Row { > 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. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 171 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line171 > > > Should this line be removed ? Increment is treated as a Mutate, so it is needed. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 208 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line208 > > > 'it's appeared' -> 'it appears' Fixed. On 2012-03-23 17:54:17, Ted Yu wrote: > src/main/proto/RegionClient.proto, line 335 > < https://reviews.apache.org/r/4054/diff/3/?file=94680#file94680line335 > > > I don't see region member in Exec.java > 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: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:20:39) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/#review6294
        -----------------------------------------------------------

        Looks good overall.
        Minor comments below.

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13644>

        Is it possible that clusterIdLeastSignificantBits is present but clusterIdMostSignificantBits is absent ?

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment13646>

        Should this rpc be called getOnlineRegions ?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13647>

        Increment doesn't extend Mutation:

        public class Increment implements Row {

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13649>

        Remove 'is'

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13651>

        white space.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13652>

        Should this line be removed ?

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13653>

        'it's appeared' -> 'it appears'

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment13654>

        I don't see region member in Exec.java
        Do we need this ?

        • Ted

        On 2012-03-22 20:20:39, Jimmy Xiang wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4054/

        -----------------------------------------------------------

        (Updated 2012-03-22 20:20:39)

        Review request for hbase.

        Summary

        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.

        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs

        -----

        pom.xml 10b13ef

        src/main/proto/RegionAdmin.proto PRE-CREATION

        src/main/proto/RegionClient.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review6294 ----------------------------------------------------------- Looks good overall. Minor comments below. src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13644 > Is it possible that clusterIdLeastSignificantBits is present but clusterIdMostSignificantBits is absent ? src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment13646 > Should this rpc be called getOnlineRegions ? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13647 > Increment doesn't extend Mutation: public class Increment implements Row { src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13649 > Remove 'is' src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13651 > white space. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13652 > Should this line be removed ? src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13653 > 'it's appeared' -> 'it appears' src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment13654 > I don't see region member in Exec.java Do we need this ? Ted On 2012-03-22 20:20:39, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:20:39) Review request for hbase. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs ----- pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12519499/hbase-5619.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 74 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12519499/hbase-5619.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 74 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1263//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/
        -----------------------------------------------------------

        (Updated 2012-03-22 20:20:39.056665)

        Review request for hbase.

        Summary (updated)
        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.
        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs


        pom.xml 10b13ef
        src/main/proto/RegionAdmin.proto PRE-CREATION
        src/main/proto/RegionClient.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:20:39.056665) Review request for hbase. Summary (updated) ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4054/
        -----------------------------------------------------------

        (Updated 2012-03-22 20:17:03.965254)

        Review request for hbase.

        Changes
        -------

        HBASE-5443 is split into individual tasks. This one is about the protocol buffer definitions: HBASE-5619

        Most of the review comments are addressed.

        Summary
        -------

        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

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5619.
        https://issues.apache.org/jira/browse/HBASE-5619

        Diffs (updated)


        pom.xml 10b13ef
        src/main/proto/RegionAdmin.proto PRE-CREATION
        src/main/proto/RegionClient.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4054/diff

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-22 20:17:03.965254) Review request for hbase. Changes ------- HBASE-5443 is split into individual tasks. This one is about the protocol buffer definitions: HBASE-5619 Most of the review comments are addressed. Summary ------- 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 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5619 . https://issues.apache.org/jira/browse/HBASE-5619 Diffs (updated) pom.xml 10b13ef src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy

          People

          • Assignee:
            Jimmy Xiang
            Reporter:
            Jimmy Xiang
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development