Details

    • Hadoop Flags:
      Incompatible change, Reviewed
    1. 5305v7.txt
      128 kB
      stack
    2. 5305v7.txt
      128 kB
      stack
    3. rpc-proto.2.txt
      25 kB
      Devaraj Das
    4. rpc-proto.3.txt
      25 kB
      Devaraj Das
    5. rpc-proto.patch.1_2
      16 kB
      Devaraj Das
    6. rpc-proto.r5.txt
      133 kB
      Devaraj Das

      Issue Links

        Activity

        Hide
        Devaraj Das added a comment -

        First pass on the patch. I still need to get the responses on protobuf.

        Show
        Devaraj Das added a comment - First pass on the patch. I still need to get the responses on protobuf.
        Hide
        stack added a comment -

        Excellent!

        Minor, would do:

        if (!head.hasUserInfo()) return;

        .. Then you'd save an indent of the whole body of the method.

        Seems like ticket should be renamed user (we seem to be creating a user rather than a ticket?) here – I like the way you ask user to create passing the header:

        • ticket = header.getUser();
          + ticket = User.create(header);

        Is ConnectionContext actually the headers? Should it be called ConnectionHeader?

        What is this – HBaseCompleteRpcRequestProto? Its 'The complete RPC request message'. Its the callid and the client request. Is it the complete request because its missing the header? Should it just be called Request since its inside a package that makes its provinence clear? I suppose request would be odd because you then do getRequest on it... hmm.

        Why tunnelRequest. Whats that mean?

        I like the builder stuff making headers and request over in client.

        Fatten doc on the proto file I'd say. Its going to be our spec.

        Can these proto classes drop the HBaseRPC prefix? Is the Proto suffix going to be our convention denoting Proto classes going forward?

        Are we doing to repeat the hrpc exception handling carrying Strings for exceptions from server to client?

        Show
        stack added a comment - Excellent! Minor, would do: if (!head.hasUserInfo()) return; .. Then you'd save an indent of the whole body of the method. Seems like ticket should be renamed user (we seem to be creating a user rather than a ticket?) here – I like the way you ask user to create passing the header: ticket = header.getUser(); + ticket = User.create(header); Is ConnectionContext actually the headers? Should it be called ConnectionHeader? What is this – HBaseCompleteRpcRequestProto? Its 'The complete RPC request message'. Its the callid and the client request. Is it the complete request because its missing the header? Should it just be called Request since its inside a package that makes its provinence clear? I suppose request would be odd because you then do getRequest on it... hmm. Why tunnelRequest. Whats that mean? I like the builder stuff making headers and request over in client. Fatten doc on the proto file I'd say. Its going to be our spec. Can these proto classes drop the HBaseRPC prefix? Is the Proto suffix going to be our convention denoting Proto classes going forward? Are we doing to repeat the hrpc exception handling carrying Strings for exceptions from server to client?
        Hide
        Devaraj Das added a comment -

        Thanks Stack, for the quick but detailed review...

        if (!head.hasUserInfo()) return;

        .. Then you'd save an indent of the whole body of the method.

        Makes sense

        Seems like ticket should be renamed user (we seem to be creating a user rather than a ticket?) here – I like the way you ask user to create passing the header:

        Makes sense

        Is ConnectionContext actually the headers? Should it be called ConnectionHeader?

        Ok

        What is this – HBaseCompleteRpcRequestProto? Its 'The complete RPC request message'. Its the callid and the client request. Is it the complete request because its missing the header? Should it just be called Request since its inside a package that makes its provinence clear? I suppose request would be odd because you then do getRequest on it... hmm.

        The CompleteRPCRequest message is composed of the RPC callID and the application RPC message (currently either a Writable or a PB). I wanted to distinguish between the two, but let me look at renaming ..

        Why tunnelRequest. Whats that mean?

        Currently, the RPC client only works with Writables. We will need to tunnel Writable RPC messages until we have PB for all the app layer protocols. Kindly have a look at the client side where the writable RPC message is serialized for sending it to the server.

        Fatten doc on the proto file I'd say. Its going to be our spec.

        Ok

        Can these proto classes drop the HBaseRPC prefix? Is the Proto suffix going to be our convention denoting Proto classes going forward?

        Will drop the prefix. But I guess the suffix should stay..

        Are we doing to repeat the hrpc exception handling carrying Strings for exceptions from server to client?

        Haven't done anything on this one yet. Let me see (this could be a separate jira IMO).

        Show
        Devaraj Das added a comment - Thanks Stack, for the quick but detailed review... if (!head.hasUserInfo()) return; .. Then you'd save an indent of the whole body of the method. Makes sense Seems like ticket should be renamed user (we seem to be creating a user rather than a ticket?) here – I like the way you ask user to create passing the header: Makes sense Is ConnectionContext actually the headers? Should it be called ConnectionHeader? Ok What is this – HBaseCompleteRpcRequestProto? Its 'The complete RPC request message'. Its the callid and the client request. Is it the complete request because its missing the header? Should it just be called Request since its inside a package that makes its provinence clear? I suppose request would be odd because you then do getRequest on it... hmm. The CompleteRPCRequest message is composed of the RPC callID and the application RPC message (currently either a Writable or a PB). I wanted to distinguish between the two, but let me look at renaming .. Why tunnelRequest. Whats that mean? Currently, the RPC client only works with Writables. We will need to tunnel Writable RPC messages until we have PB for all the app layer protocols. Kindly have a look at the client side where the writable RPC message is serialized for sending it to the server. Fatten doc on the proto file I'd say. Its going to be our spec. Ok Can these proto classes drop the HBaseRPC prefix? Is the Proto suffix going to be our convention denoting Proto classes going forward? Will drop the prefix. But I guess the suffix should stay.. Are we doing to repeat the hrpc exception handling carrying Strings for exceptions from server to client? Haven't done anything on this one yet. Let me see (this could be a separate jira IMO).
        Hide
        stack added a comment -

        Ok on the tunnel thing. Maybe comment it some more (if you haven't already) in code.

        Yeah on suffix. We need convention I'd say distingushing the PB classes.

        On exception, could do as separate jira. Here is one that looks like its what you need that already exists, if it helps: HBASE-2030

        Show
        stack added a comment - Ok on the tunnel thing. Maybe comment it some more (if you haven't already) in code. Yeah on suffix. We need convention I'd say distingushing the PB classes. On exception, could do as separate jira. Here is one that looks like its what you need that already exists, if it helps: HBASE-2030
        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- Review request for hbase. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1293032 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1293032 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1293032 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1293032 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1293032 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        Devaraj Das added a comment -

        Updated reviewboard patch has most of the review comments taken care of i think.

        Show
        Devaraj Das added a comment - Updated reviewboard patch has most of the review comments taken care of i think.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-03-01 03:40:14.496251)

        Review request for .

        Changes
        -------

        Updated with more doc.

        Summary
        -------

        Switch RPC call envelope/headers to PBs

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

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing
        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14.496251) Review request for . Changes ------- Updated with more doc. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        Devaraj Das added a comment -

        Uploading patch so I can submit this to hudson.

        Show
        Devaraj Das added a comment - Uploading patch so I can submit this to hudson.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516704/rpc-proto.2.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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1073//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/12516704/rpc-proto.2.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1073//console This message is automatically generated.
        Hide
        Devaraj Das added a comment -

        Patch merged with current trunk.

        Show
        Devaraj Das added a comment - Patch merged with current trunk.
        Hide
        stack added a comment -

        I wonder how Jimmy is doing stuff like the below:

        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.ConnectionHeaderProto;
        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcRequestWithHeaderProto;
        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcResponseWithHeaderProto;
        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcExceptionProto;
        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcRequestProto;
        +import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcResponseProto;
        

        ... over in his patch (he's just done the proto stuff so far – not sure about what the generated stuff will look like just yet).

        We need to have convention around this stuff. Might be worth chat up on dev list.

        For example, I wonder if we need the protobuf package here since all proto classes are contained in RPCMessageProtos (and its got a suffix to identify it as proto generated).

        The below is a pity but as you say, shouldn't live long:

        +      result.write(d);
        +      //makes a copy; but this part of code is not going to live long
        +      //hopefully (only until we move all the protocols to protobuf)
        +      response.setResponse(ByteString.copyFrom(d.getData()));
        

        Is that maybeTranslate method repeated?

        Exceptions still strings then?

        + response.getException().getStackTrace()));

        Looks like your pom edit clashes with Jimmys over in the HRegionInterface redo. May the first commit win.

        The doc on the proto file is great.

        This is shaping up nice.

        Jimmy, you should review!

        Show
        stack added a comment - I wonder how Jimmy is doing stuff like the below: + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.ConnectionHeaderProto; + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcRequestWithHeaderProto; + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcResponseWithHeaderProto; + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcExceptionProto; + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcRequestProto; + import org.apache.hadoop.hbase.ipc.protobuf.RPCMessageProtos.RpcResponseProto; ... over in his patch (he's just done the proto stuff so far – not sure about what the generated stuff will look like just yet). We need to have convention around this stuff. Might be worth chat up on dev list. For example, I wonder if we need the protobuf package here since all proto classes are contained in RPCMessageProtos (and its got a suffix to identify it as proto generated). The below is a pity but as you say, shouldn't live long: + result.write(d); + //makes a copy; but this part of code is not going to live long + //hopefully (only until we move all the protocols to protobuf) + response.setResponse(ByteString.copyFrom(d.getData())); Is that maybeTranslate method repeated? Exceptions still strings then? + response.getException().getStackTrace())); Looks like your pom edit clashes with Jimmys over in the HRegionInterface redo. May the first commit win. The doc on the proto file is great. This is shaping up nice. Jimmy, you should review!
        Hide
        Devaraj Das added a comment -

        For example, I wonder if we need the protobuf package here since all proto classes are contained in RPCMessageProtos (and its got a suffix to identify it as proto generated).

        Ok. I will go with what everyone thinks. I am pretty unbiased..

        Is that maybeTranslate method repeated?

        Yes, but with different semantics. Again, these translate methods are required for the interim only (as long as we have both writable rpc engine and protobuf rpc engine). Since the protobuf rpc engine is not there yet, we have to always do the translation.

        Exceptions still strings then?

        Currently the exceptions are protobuf objs with exception-name and exception-trace as the fields. I think this is the closest we can get if we need to support multiple languages on the client side over protobuf. If we have Java clients, we can do the forname trick to construct an instance of the exception class and fill up the exception trace, etc. but this can be addressed in a separate jira. Makes sense?

        Show
        Devaraj Das added a comment - For example, I wonder if we need the protobuf package here since all proto classes are contained in RPCMessageProtos (and its got a suffix to identify it as proto generated). Ok. I will go with what everyone thinks. I am pretty unbiased.. Is that maybeTranslate method repeated? Yes, but with different semantics. Again, these translate methods are required for the interim only (as long as we have both writable rpc engine and protobuf rpc engine). Since the protobuf rpc engine is not there yet, we have to always do the translation. Exceptions still strings then? Currently the exceptions are protobuf objs with exception-name and exception-trace as the fields. I think this is the closest we can get if we need to support multiple languages on the client side over protobuf. If we have Java clients, we can do the forname trick to construct an instance of the exception class and fill up the exception trace, etc. but this can be addressed in a separate jira. Makes sense?
        Hide
        Jimmy Xiang added a comment -

        I did a quick review last night. Looks ok with me.
        For the pom change, we have the same change. So it should be fine.

        For me, I put the generated files under org.apache.hadoop.hbase.protobuf.
        Should I put them under org.apache.hadoop.hbase.ipc.protobuf too?

        Show
        Jimmy Xiang added a comment - I did a quick review last night. Looks ok with me. For the pom change, we have the same change. So it should be fine. For me, I put the generated files under org.apache.hadoop.hbase.protobuf. Should I put them under org.apache.hadoop.hbase.ipc.protobuf too?
        Hide
        Todd Lipcon added a comment -

        Can we avoid the copy in the interim by having a convention that, if the request is a protobuf, then we send it following the call envelope rather than inside it? (does that make sense?)

        Show
        Todd Lipcon added a comment - Can we avoid the copy in the interim by having a convention that, if the request is a protobuf, then we send it following the call envelope rather than inside it? (does that make sense?)
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment11928>

        Should we them under
        org.apache.hadoop.hbase.ipc.protobuf.generated, or org.apache.hadoop.hbase.protobuf.generated, since they are generated classes?

        • Jimmy

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review5509 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment11928 > Should we them under org.apache.hadoop.hbase.ipc.protobuf.generated, or org.apache.hadoop.hbase.protobuf.generated, since they are generated classes? Jimmy On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516705/rpc-proto.3.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 appears to have generated -131 warning messages.

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

        -1 findbugs. The patch appears to introduce 165 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.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//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/12516705/rpc-proto.3.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 appears to have generated -131 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 165 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.coprocessor.TestMasterObserver org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1074//console This message is automatically generated.
        Hide
        Devaraj Das added a comment -

        Can we avoid the copy in the interim by having a convention that, if the request is a protobuf, then we send it following the call envelope rather than inside it? (does that make sense?)

        I explored this route but seems like it's not straightforward to do this (due to the fact that there are assumptions made on the order of <data-length> and <data> on the server, and I'd have to make changes to that to accommodate sending another set of bytes after the call envelope .. messy). I propose we leave the "copy" around and fix it by introducing something similar to ProtobufRpcEngine (of Hadoop) that would use native PBs everywhere. Of course we have to complete moving all protocols to PB.
        If people agree with me, I can submit a patch with only the path for the generated classes changed to what Jimmy suggested.

        Thoughts?

        Show
        Devaraj Das added a comment - Can we avoid the copy in the interim by having a convention that, if the request is a protobuf, then we send it following the call envelope rather than inside it? (does that make sense?) I explored this route but seems like it's not straightforward to do this (due to the fact that there are assumptions made on the order of <data-length> and <data> on the server, and I'd have to make changes to that to accommodate sending another set of bytes after the call envelope .. messy). I propose we leave the "copy" around and fix it by introducing something similar to ProtobufRpcEngine (of Hadoop) that would use native PBs everywhere. Of course we have to complete moving all protocols to PB. If people agree with me, I can submit a patch with only the path for the generated classes changed to what Jimmy suggested. Thoughts?
        Hide
        Todd Lipcon added a comment -

        Sounds reasonable, thanks Devaraj.

        Show
        Todd Lipcon added a comment - Sounds reasonable, thanks Devaraj.
        Hide
        stack added a comment -

        Can we have hbase go all-pb for hbase 0.96.0?

        Show
        stack added a comment - Can we have hbase go all-pb for hbase 0.96.0?
        Hide
        stack added a comment -

        Thats a dumb question. Let me rephrase. Won't hbase be all pb natively by 0.96.0?

        Show
        stack added a comment - Thats a dumb question. Let me rephrase. Won't hbase be all pb natively by 0.96.0?
        Hide
        Devaraj Das added a comment -

        Won't hbase be all pb natively by 0.96.0?

        That should be the goal, rather a blocker smile

        Show
        Devaraj Das added a comment - Won't hbase be all pb natively by 0.96.0? That should be the goal, rather a blocker smile
        Hide
        Jimmy Xiang added a comment -

        I hope we can. I know the RPC won't be backward compatible. How about the client code? We definitely won't break any existing client applications, right?

        Show
        Jimmy Xiang added a comment - I hope we can. I know the RPC won't be backward compatible. How about the client code? We definitely won't break any existing client applications, right?
        Hide
        stack added a comment -

        Client APIs should be the same but yeah, lets get up on pb before 0.96.0; a blocker as per DD.

        Show
        stack added a comment - Client APIs should be the same but yeah, lets get up on pb before 0.96.0; a blocker as per DD.
        Hide
        Devaraj Das added a comment -

        I'll wait for the patch for HBASE-5443 to be committed before I submit another patch on this. The pom.xml update on HBASE-5443 is under discussion and since this patch makes similar updates to pom.xml the discussion impacts this jira too.

        Show
        Devaraj Das added a comment - I'll wait for the patch for HBASE-5443 to be committed before I submit another patch on this. The pom.xml update on HBASE-5443 is under discussion and since this patch makes similar updates to pom.xml the discussion impacts this jira too.
        Hide
        Devaraj Das added a comment -

        After talking with Jimmy offline, it seems that this patch shouldn't wait for HBASE-5443. The patch here is ready to go whereas HBASE-5443 has work, other than the pom.xml stuff, yet to be done. Shall we look at committing this patch (with the pom.xml as is), and then address pom.xml changes in a separate jira or in HBASE-5443.

        Show
        Devaraj Das added a comment - After talking with Jimmy offline, it seems that this patch shouldn't wait for HBASE-5443 . The patch here is ready to go whereas HBASE-5443 has work, other than the pom.xml stuff, yet to be done. Shall we look at committing this patch (with the pom.xml as is), and then address pom.xml changes in a separate jira or in HBASE-5443 .
        Hide
        Devaraj Das added a comment -

        Could someone please look at this. Thanks!

        Show
        Devaraj Das added a comment - Could someone please look at this. Thanks!
        Hide
        stack added a comment -

        Sorry DD. Trying to get Benoit to review it before committing. I've given him his requisite $3.00 but I might have to give him more to bump me up in his queue.

        Show
        stack added a comment - Sorry DD. Trying to get Benoit to review it before committing. I've given him his requisite $3.00 but I might have to give him more to bump me up in his queue.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml
        <https://reviews.apache.org/r/4096/#comment13673>

        Trailing whitespaces.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment13674>

        You already import RpcRequestWithHeaderProto, so just use "RpcRequestWithHeaderProto.Builder" here, drop the leading "RPCMessageProtos."

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment13675>

        Trailing whitespaces here and below. Kill them all.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment13677>

        If you cast it to RpcRequestProto, then why not check if param is an instance of RpcRequestProto? Also you're missing a space right before "param".

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment13678>

        Throw a ClassCastException.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/4096/#comment13679>

        Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/4096/#comment13680>

        Trailing whitespace.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/4096/#comment13739>

        Is there a way to avoid code duplication and unify this method with the on in the HBaseClient class?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/4096/#comment13740>

        Why wrap this line and the next around? I think this fits on one line without exceeding 80 columns.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        <https://reviews.apache.org/r/4096/#comment13670>

        just do return create(null) ?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        <https://reviews.apache.org/r/4096/#comment13671>

        Why use the fully qualified names here?

        Also kill the trailing whitespace.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        <https://reviews.apache.org/r/4096/#comment13672>

        Trailing whitespaces.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        <https://reviews.apache.org/r/4096/#comment13741>

        This seems unnecessary.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13742>

        Kill all the trailing whitespaces!

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13743>

        I don't see how this is graceful.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13744>

        Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13745>

        What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13746>

        Why is this optional?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13747>

        Why is this optional? It should be required and it should be first.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13748>

        Ditto, why have an extra PB?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13749>

        This should be first and it should be required.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment13750>

        This should also be required.

        • Benoit

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml < https://reviews.apache.org/r/4096/#comment13673 > Trailing whitespaces. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment13674 > You already import RpcRequestWithHeaderProto, so just use "RpcRequestWithHeaderProto.Builder" here, drop the leading "RPCMessageProtos." http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment13675 > Trailing whitespaces here and below. Kill them all. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment13677 > If you cast it to RpcRequestProto, then why not check if param is an instance of RpcRequestProto? Also you're missing a space right before "param". http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment13678 > Throw a ClassCastException. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/4096/#comment13679 > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/4096/#comment13680 > Trailing whitespace. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/4096/#comment13739 > Is there a way to avoid code duplication and unify this method with the on in the HBaseClient class? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/4096/#comment13740 > Why wrap this line and the next around? I think this fits on one line without exceeding 80 columns. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java < https://reviews.apache.org/r/4096/#comment13670 > just do return create(null) ? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java < https://reviews.apache.org/r/4096/#comment13671 > Why use the fully qualified names here? Also kill the trailing whitespace. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java < https://reviews.apache.org/r/4096/#comment13672 > Trailing whitespaces. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java < https://reviews.apache.org/r/4096/#comment13741 > This seems unnecessary. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13742 > Kill all the trailing whitespaces! http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13743 > I don't see how this is graceful. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13744 > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13745 > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13746 > Why is this optional? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13747 > Why is this optional? It should be required and it should be first. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13748 > Ditto, why have an extra PB? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13749 > This should be first and it should be required. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment13750 > This should also be required. Benoit On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version.

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 34

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34>

        >

        > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface.

        Note that this is just documentation. Ping is already done in hbase RPC, and I thought I'd document it. I haven't done anything in the PB stuff for handling this. I agree with you this is odd/special-case and IMO a topic for a separate jira.

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>

        >

        > Why is this optional?

        General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense?

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>

        >

        > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?

        The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects.

        Similarly, on the response side.

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 25

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line25>

        >

        > I don't see how this is graceful.

        I answered this above.

        • Devaraj

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version. On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 34 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34 > > > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface. Note that this is just documentation. Ping is already done in hbase RPC, and I thought I'd document it. I haven't done anything in the PB stuff for handling this. I agree with you this is odd/special-case and IMO a topic for a separate jira. On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 72 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72 > > > Why is this optional? General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense? On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 71 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71 > > > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it? The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects. Similarly, on the response side. On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 25 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line25 > > > I don't see how this is graceful. I answered this above. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        Devaraj Das wrote:

        I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version.

        Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting.

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 34

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34>

        >

        > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface.

        Devaraj Das wrote:

        Note that this is just documentation. Ping is already done in hbase RPC, and I thought I'd document it. I haven't done anything in the PB stuff for handling this. I agree with you this is odd/special-case and IMO a topic for a separate jira.

        Separate jira fine.

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>

        >

        > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?

        Devaraj Das wrote:

        The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects.

        Similarly, on the response side.

        How hard to leave it out DD and add later if we need it?

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>

        >

        > Why is this optional?

        Devaraj Das wrote:

        General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense?

        Sure in general. What about the specific comment? Seems like its required?

        • Michael

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. Devaraj Das wrote: I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version. Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting. On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 34 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34 > > > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface. Devaraj Das wrote: Note that this is just documentation. Ping is already done in hbase RPC, and I thought I'd document it. I haven't done anything in the PB stuff for handling this. I agree with you this is odd/special-case and IMO a topic for a separate jira. Separate jira fine. On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 71 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71 > > > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it? Devaraj Das wrote: The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects. Similarly, on the response side. How hard to leave it out DD and add later if we need it? On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 72 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72 > > > Why is this optional? Devaraj Das wrote: General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense? Sure in general. What about the specific comment? Seems like its required? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>

        >

        > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?

        Devaraj Das wrote:

        The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects.

        Similarly, on the response side.

        Michael Stack wrote:

        How hard to leave it out DD and add later if we need it?

        I'll check on whether the current stuff can be moved into one PB cleanly (but again in the near future we'll need to break it up into two as per my current thinking of how things will be implemented).

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72

        > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>

        >

        > Why is this optional?

        Devaraj Das wrote:

        General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense?

        Michael Stack wrote:

        Sure in general. What about the specific comment? Seems like its required?

        Ok.. I'll make the critical fields "REQUIRED"

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        Devaraj Das wrote:

        I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version.

        Michael Stack wrote:

        Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting.

        Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.

        • Devaraj

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 71 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71 > > > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it? Devaraj Das wrote: The main reason being I wanted to clearly separate what comes from the application and what's put in by the RPC layer. The client would frame a PB object (RpcRequestProto) and send it down to the RPC layer. Currently, the RpcRequestProto is mostly a placeholder with only one field called 'bytes'. Once I implement the ProtoBufRpcEngine (as in Hadoop core) in a follow-up jira, I'll have fields like "methodname', 'protocolname', etc. and they would be encoded as RpcRequestProto objects. Similarly, on the response side. Michael Stack wrote: How hard to leave it out DD and add later if we need it? I'll check on whether the current stuff can be moved into one PB cleanly (but again in the near future we'll need to break it up into two as per my current thinking of how things will be implemented). On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto , line 72 > < https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72 > > > Why is this optional? Devaraj Das wrote: General comment on the optional vs required PB fields... I have made most of the fields as optional since it makes the specification flexible and makes compatibility very easy. Once we are somewhat certain of the PB fields in the RPC we can finalize on the labeling of optional/required on the fields. Does this make sense? Michael Stack wrote: Sure in general. What about the specific comment? Seems like its required? Ok.. I'll make the critical fields "REQUIRED" On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. Devaraj Das wrote: I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version. Michael Stack wrote: Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting. Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        Devaraj Das wrote:

        I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version.

        Michael Stack wrote:

        Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting.

        Devaraj Das wrote:

        Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.

        On ..."The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.", its useless as is. Can we make this rationale? Like if version is bumped, it tells client what version server is?

        • Michael

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. Devaraj Das wrote: I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version. Michael Stack wrote: Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting. Devaraj Das wrote: Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know. On ..."The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.", its useless as is. Can we make this rationale? Like if version is bumped, it tells client what version server is? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-24 07:38:03, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated.

        Devaraj Das wrote:

        I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version.

        Michael Stack wrote:

        Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting.

        Devaraj Das wrote:

        Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.

        Michael Stack wrote:

        On ..."The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.", its useless as is. Can we make this rationale? Like if version is bumped, it tells client what version server is?

        Hi Stack, not sure what you meant in your last comment. The VersionMismatch exception that is sent to the client has an accompanying message that says something like - "Server IPC version <current-version> cannot communicate .. ". By parsing the exception the client can know what's wrong (hacky but works).

        Once we have PB in the RPC we can actually remove this version check since clients/servers talk PB and PB will handle compatibility in the RPC messages. But I want to change things with more thought and as such want to keep the version number around for at least this jira.

        Given the above, I am not sure what to do: to me version change seems sufficient to catch non-compliant clients early (and since the RPC is changing in a major by switching to PB, makes sense to me to change the version number). If on the other hand, we let the client pass this initial step by not changing the version number, we'll let old clients pass this initial step. It'll fail later on.

        Thoughts?

        • Devaraj

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-24 07:38:03, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. Devaraj Das wrote: I see. This was the basis of the "graceful" failure for current clients that are not aware of PB (clients would bail out if the versions of RPC don't match, right). The response to your comment below "I don't see how this is graceful." is actually this change in the version. Michael Stack wrote: Benoit's point is that this mechanism doesn't work so his point is lets not bother changing the version. Previous, if you volunteered a hrpc version other than what is expected, the connection was closed by the server w/o saying what was wrong. We fixed hbase so it at least throws an exception but it doesn't say what version its expecting. Devaraj Das wrote: Stack, if we don't change the server version number then even the exception you're referring to won't be thrown. The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know. Michael Stack wrote: On ..."The exception/error will happen later on in the processing of the RPC... Are we sure we want this as the behavior? Please let me know.", its useless as is. Can we make this rationale? Like if version is bumped, it tells client what version server is? Hi Stack, not sure what you meant in your last comment. The VersionMismatch exception that is sent to the client has an accompanying message that says something like - "Server IPC version <current-version> cannot communicate .. ". By parsing the exception the client can know what's wrong (hacky but works). Once we have PB in the RPC we can actually remove this version check since clients/servers talk PB and PB will handle compatibility in the RPC messages. But I want to change things with more thought and as such want to keep the version number around for at least this jira. Given the above, I am not sure what to do: to me version change seems sufficient to catch non-compliant clients early (and since the RPC is changing in a major by switching to PB, makes sense to me to change the version number). If on the other hand, we let the client pass this initial step by not changing the version number, we'll let old clients pass this initial step. It'll fail later on. Thoughts? Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6302 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        <https://reviews.apache.org/r/4096/#comment14193>

        Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is.

        I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT.

        This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that.

        • Benoit

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6525 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java < https://reviews.apache.org/r/4096/#comment14193 > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is. I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT . This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that. Benoit On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-29 18:13:02, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is.

        >

        > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT.

        >

        > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that.

        Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something?

        I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later).

        • Devaraj

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-29 18:13:02, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is. > > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT . > > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that. Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something? I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later). Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6525 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-29 18:13:02, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is.

        >

        > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT.

        >

        > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that.

        Devaraj Das wrote:

        Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something?

        I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later).

        Deveraj: I talked w/ B. It makes sense that you be able to find the 'version' of an hbase cluster, or at least, the version that a client should use when it goes to read the root/meta region content by looking in zk. I intend to remove root for 0.96.0. I also intend to change how all is serialized to zk in 0.96 to make it pb based. When I change the root-region-location in zk, I'll include version a client needs reading (talking w/ Benoit, rather than remove this znode, we should probably just keep it only have it point at .META. from here on out; i.e. meta becomes the root.. but that is for another issue).

        So, go ahead, please change the version in your patch. Sorry for the distraction.

        • Michael

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-29 18:13:02, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is. > > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT . > > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that. Devaraj Das wrote: Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something? I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later). Deveraj: I talked w/ B. It makes sense that you be able to find the 'version' of an hbase cluster, or at least, the version that a client should use when it goes to read the root/meta region content by looking in zk. I intend to remove root for 0.96.0. I also intend to change how all is serialized to zk in 0.96 to make it pb based. When I change the root-region-location in zk, I'll include version a client needs reading (talking w/ Benoit, rather than remove this znode, we should probably just keep it only have it point at .META. from here on out; i.e. meta becomes the root.. but that is for another issue). So, go ahead, please change the version in your patch. Sorry for the distraction. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6525 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-29 18:13:02, Benoit Sigoure wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102

        > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>

        >

        > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is.

        >

        > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT.

        >

        > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that.

        Devaraj Das wrote:

        Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something?

        I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later).

        Michael Stack wrote:

        Deveraj: I talked w/ B. It makes sense that you be able to find the 'version' of an hbase cluster, or at least, the version that a client should use when it goes to read the root/meta region content by looking in zk. I intend to remove root for 0.96.0. I also intend to change how all is serialized to zk in 0.96 to make it pb based. When I change the root-region-location in zk, I'll include version a client needs reading (talking w/ Benoit, rather than remove this znode, we should probably just keep it only have it point at .META. from here on out; i.e. meta becomes the root.. but that is for another issue).

        So, go ahead, please change the version in your patch. Sorry for the distraction.

        Thanks, Stack, for helping on getting a resolution!

        Is there a jira on the topic of removing root for 0.96.0 ?

        • Devaraj

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

        On 2012-03-01 03:40:14, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-01 03:40:14)

        Review request for .

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-29 18:13:02, Benoit Sigoure wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java , line 102 > < https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102 > > > Devaraj, the problem with up'ing the version number is that it makes the life of backwards-compatible clients like asynchbase even more difficult than it already is. > > I proposed another idea to Stack, I don't know if he shared it with you, so here it is: In the znode used to store the ROOT region, add the protocol version number (make it 5 there if you want). This way clients that are finding where ROOT is will be able to figure out the protocol version to use before connecting to ROOT . > > This better than parsing the string of the VersionMismatchException, which you said yourself is hacky (and also inefficient), so we don't wanna do that. Devaraj Das wrote: Thanks, Benoit for getting back. The idea you have is cool but I don't see how that'd apply to old existing clients. In the asynchbase case, one would have to write new code to take care of the proposed arrangement, right? Am I missing something? I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later). Michael Stack wrote: Deveraj: I talked w/ B. It makes sense that you be able to find the 'version' of an hbase cluster, or at least, the version that a client should use when it goes to read the root/meta region content by looking in zk. I intend to remove root for 0.96.0. I also intend to change how all is serialized to zk in 0.96 to make it pb based. When I change the root-region-location in zk, I'll include version a client needs reading (talking w/ Benoit, rather than remove this znode, we should probably just keep it only have it point at .META. from here on out; i.e. meta becomes the root.. but that is for another issue). So, go ahead, please change the version in your patch. Sorry for the distraction. Thanks, Stack, for helping on getting a resolution! Is there a jira on the topic of removing root for 0.96.0 ? Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6525 ----------------------------------------------------------- On 2012-03-01 03:40:14, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-01 03:40:14) Review request for . Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-03-30 23:29:32.254174)

        Review request for Michael Stack and Benoit Sigoure.

        Changes
        -------

        This should take care of the comments.

        Summary
        -------

        Switch RPC call envelope/headers to PBs

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

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing
        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32.254174) Review request for Michael Stack and Benoit Sigoure. Changes ------- This should take care of the comments. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12520689/rpc-proto.r5.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 79 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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//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/12520689/rpc-proto.r5.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 79 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1357//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/4096/#review6613
        -----------------------------------------------------------

        Some more questions. Just being careful DD.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java
        <https://reviews.apache.org/r/4096/#comment14285>

        We should just be using the hadoop DOOS... looks like no diff (when I diff them). I'll make an issue to remove.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment14278>

        Is this written up anywhere? That its hrpc, then version, then a length, then a protobuf?

        I see it in the proto definition. That'll do.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        <https://reviews.apache.org/r/4096/#comment14279>

        We have an issue for removing this Invocation stuff?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment14280>

        Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients?

        What is 'failure compatibility'? We are telling the client to go away, nicely (smile).

        What you think we should replace hrpc0x0005 with?

        this -> these

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment14281>

        How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say?

        Would be nice to have illustration on how the back and forth work.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment14282>

        We'll send this String each time?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment14283>

        Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto
        <https://reviews.apache.org/r/4096/#comment14284>

        Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        • Michael

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- Some more questions. Just being careful DD. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java < https://reviews.apache.org/r/4096/#comment14285 > We should just be using the hadoop DOOS... looks like no diff (when I diff them). I'll make an issue to remove. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment14278 > Is this written up anywhere? That its hrpc, then version, then a length, then a protobuf? I see it in the proto definition. That'll do. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java < https://reviews.apache.org/r/4096/#comment14279 > We have an issue for removing this Invocation stuff? http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment14280 > Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients? What is 'failure compatibility'? We are telling the client to go away, nicely (smile). What you think we should replace hrpc0x0005 with? this -> these http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment14281 > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say? Would be nice to have illustration on how the back and forth work. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment14282 > We'll send this String each time? http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment14283 > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto < https://reviews.apache.org/r/4096/#comment14284 > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Michael On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > Some more questions. Just being careful DD.

        That's fine. Hope the answers below are okay. Please let me know your response soon so that I can submit another patch.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java, line 25

        > <https://reviews.apache.org/r/4096/diff/3/?file=97739#file97739line25>

        >

        > We should just be using the hadoop DOOS... looks like no diff (when I diff them). I'll make an issue to remove.

        cool

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 446

        > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line446>

        >

        > Is this written up anywhere? That its hrpc, then version, then a length, then a protobuf?

        >

        > I see it in the proto definition. That'll do.

        cool

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 548

        > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548>

        >

        > We have an issue for removing this Invocation stuff?

        No not yet. But I'll create one to do with this issue once this patch is committed.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 25

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25>

        >

        > Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients?

        >

        > What is 'failure compatibility'? We are telling the client to go away, nicely (smile).

        >

        > What you think we should replace hrpc0x0005 with?

        >

        > this -> these

        Yeah, valid points. We can remove this version string and all in a follow up patch.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 28

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28>

        >

        > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say?

        >

        > Would be nice to have illustration on how the back and forth work.

        The latter is used only while establishing connections and the former for exchanging RPC requests/responses over a channel that is connected. Okay, will add some text.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 55

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55>

        >

        > We'll send this String each time?

        Actually, I could make this field 'optional' since this has a default value. Will do so.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>

        >

        > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        >

        > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know.

        'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it..

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        • Devaraj

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > Some more questions. Just being careful DD. That's fine. Hope the answers below are okay. Please let me know your response soon so that I can submit another patch. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java , line 25 > < https://reviews.apache.org/r/4096/diff/3/?file=97739#file97739line25 > > > We should just be using the hadoop DOOS... looks like no diff (when I diff them). I'll make an issue to remove. cool On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java , line 446 > < https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line446 > > > Is this written up anywhere? That its hrpc, then version, then a length, then a protobuf? > > I see it in the proto definition. That'll do. cool On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java , line 548 > < https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548 > > > We have an issue for removing this Invocation stuff? No not yet. But I'll create one to do with this issue once this patch is committed. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 25 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25 > > > Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients? > > What is 'failure compatibility'? We are telling the client to go away, nicely (smile). > > What you think we should replace hrpc0x0005 with? > > this -> these Yeah, valid points. We can remove this version string and all in a follow up patch. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 28 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28 > > > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say? > > Would be nice to have illustration on how the back and forth work. The latter is used only while establishing connections and the former for exchanging RPC requests/responses over a channel that is connected. Okay, will add some text. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 55 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55 > > > We'll send this String each time? Actually, I could make this field 'optional' since this has a default value. Will do so. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 66 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66 > > > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? > > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know. 'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it.. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 548

        > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548>

        >

        > We have an issue for removing this Invocation stuff?

        Devaraj Das wrote:

        No not yet. But I'll create one to do with this issue once this patch is committed.

        Thanks

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 25

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25>

        >

        > Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients?

        >

        > What is 'failure compatibility'? We are telling the client to go away, nicely (smile).

        >

        > What you think we should replace hrpc0x0005 with?

        >

        > this -> these

        Devaraj Das wrote:

        Yeah, valid points. We can remove this version string and all in a follow up patch.

        Lets discuss in another jira. A <hrpc> <version> followed by something that says its protobuf that follows, etc.,

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 28

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28>

        >

        > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say?

        >

        > Would be nice to have illustration on how the back and forth work.

        Devaraj Das wrote:

        The latter is used only while establishing connections and the former for exchanging RPC requests/responses over a channel that is connected. Okay, will add some text.

        Thanks

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 55

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55>

        >

        > We'll send this String each time?

        Devaraj Das wrote:

        Actually, I could make this field 'optional' since this has a default value. Will do so.

        That'd be a good idea I think. The other protocols are less used and can include the String

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>

        >

        > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        >

        > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        Devaraj Das wrote:

        Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know.

        'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it..

        I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement?

        On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        • Michael

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java , line 548 > < https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548 > > > We have an issue for removing this Invocation stuff? Devaraj Das wrote: No not yet. But I'll create one to do with this issue once this patch is committed. Thanks On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 25 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25 > > > Should we just remove them in the next iteration on rpc since 0.96 is to be a singularity? Why even bother trying to keep compatibility w/ older clients? > > What is 'failure compatibility'? We are telling the client to go away, nicely (smile). > > What you think we should replace hrpc0x0005 with? > > this -> these Devaraj Das wrote: Yeah, valid points. We can remove this version string and all in a follow up patch. Lets discuss in another jira. A <hrpc> <version> followed by something that says its protobuf that follows, etc., On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 28 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28 > > > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say? > > Would be nice to have illustration on how the back and forth work. Devaraj Das wrote: The latter is used only while establishing connections and the former for exchanging RPC requests/responses over a channel that is connected. Okay, will add some text. Thanks On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 55 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55 > > > We'll send this String each time? Devaraj Das wrote: Actually, I could make this field 'optional' since this has a default value. Will do so. That'd be a good idea I think. The other protocols are less used and can include the String On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 66 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66 > > > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? > > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? Devaraj Das wrote: Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know. 'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it.. I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement? On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        The argument above for 'length' applies here too...

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>

        >

        > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        >

        > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        Devaraj Das wrote:

        Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know.

        'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it..

        Michael Stack wrote:

        I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement?

        On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say.

        Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional.

        Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense?

        (Also note that the top level RPC request envelope has the length preceding the request data)

        • Devaraj

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). The argument above for 'length' applies here too... On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 66 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66 > > > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? > > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? Devaraj Das wrote: Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know. 'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it.. Michael Stack wrote: I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement? On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say. Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional. Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense? (Also note that the top level RPC request envelope has the length preceding the request data) Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>

        >

        > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        >

        > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        Devaraj Das wrote:

        Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know.

        'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it..

        Michael Stack wrote:

        I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement?

        On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say.

        Devaraj Das wrote:

        Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional.

        Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense?

        (Also note that the top level RPC request envelope has the length preceding the request data)

        If the top level rpc request envelope has the length, then I agree w/ you, its not needed as prefix on pb messages.

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        • Michael

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 66 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66 > > > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? > > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? Devaraj Das wrote: Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know. 'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it.. Michael Stack wrote: I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement? On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say. Devaraj Das wrote: Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional. Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense? (Also note that the top level RPC request envelope has the length preceding the request data) If the top level rpc request envelope has the length, then I agree w/ you, its not needed as prefix on pb messages. On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>

        >

        > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?

        >

        > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?

        Devaraj Das wrote:

        Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know.

        'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it..

        Michael Stack wrote:

        I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement?

        On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say.

        Devaraj Das wrote:

        Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional.

        Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense?

        (Also note that the top level RPC request envelope has the length preceding the request data)

        Michael Stack wrote:

        If the top level rpc request envelope has the length, then I agree w/ you, its not needed as prefix on pb messages.

        cool

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Michael Stack wrote:

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        I meant the argument on the PB encoding..

        The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId.

        • Devaraj

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 66 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66 > > > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto? > > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here? Devaraj Das wrote: Today the only 'header' is the callId.. There is no relation to ConnectionHeaderProto. If the 'header' is confusing, I can take it off the object name. Let me know. 'request' in this patch is only a Invocation/Writable. In theory, it could be a protobuf object as well (since it is just bytes), but, for protobuf, we could make things more explicit by defining a protobuf object rather than a opaque set of bytes. But that's another jira (ProtoBufRpcEngine implementation similar to Hadoop). Length is not needed - the protobuf serialization/deserialization will take care of it.. Michael Stack wrote: I think taking the 'Header' off Request/Response would be best (Did I ask you add it previous? If so, sorry... I misunderstood. Thanks for being accomodating). Yes, on a new issue to make it pb rather than opaque bytes. Do you have to do something here – make bytes optional? – to allow for the later pb replacement? On length, thats probably good to keep. For us, we'll give the stream to a pb deserializer but other clients might want to know how many bytes on the line.... so keep it I'd say. Devaraj Das wrote: Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional. Actually, on the length, I am not sure I understand why we need it in the PB model. Generally speaking, clients talking to servers have to be aware of the PB encoding in order for them to make any sense of the PB data.. The PB type 'bytes' has the length taken care of in the implementation of serialization/deserialization internally. In that sense, I don't think having an explicit length field is required. Does this reasoning make sense? (Also note that the top level RPC request envelope has the length preceding the request data) Michael Stack wrote: If the top level rpc request envelope has the length, then I agree w/ you, its not needed as prefix on pb messages. cool On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Michael Stack wrote: Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. I meant the argument on the PB encoding.. The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Michael Stack wrote:

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        Devaraj Das wrote:

        I meant the argument on the PB encoding..

        The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId.

        Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.

        • Michael

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Michael Stack wrote: Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. Devaraj Das wrote: I meant the argument on the PB encoding.. The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId. Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Michael Stack wrote:

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        Devaraj Das wrote:

        I meant the argument on the PB encoding..

        The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId.

        Michael Stack wrote:

        Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.

        Okay let's discuss that in a separate jira..

        Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated.

        • Devaraj

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Michael Stack wrote: Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. Devaraj Das wrote: I meant the argument on the PB encoding.. The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId. Michael Stack wrote: Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix. Okay let's discuss that in a separate jira.. Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Michael Stack wrote:

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        Devaraj Das wrote:

        I meant the argument on the PB encoding..

        The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId.

        Michael Stack wrote:

        Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.

        Devaraj Das wrote:

        Okay let's discuss that in a separate jira..

        Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated.

        There items above you said you'd address such as removing Header from the request and response and cleaning up doc in the .proto file, right?

        • Michael

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Michael Stack wrote: Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. Devaraj Das wrote: I meant the argument on the PB encoding.. The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId. Michael Stack wrote: Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix. Devaraj Das wrote: Okay let's discuss that in a separate jira.. Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated. There items above you said you'd address such as removing Header from the request and response and cleaning up doc in the .proto file, right? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-02 00:21:20, Michael Stack wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93

        > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>

        >

        > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too?

        Devaraj Das wrote:

        Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know.

        Michael Stack wrote:

        Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull).

        Devaraj Das wrote:

        The argument above for 'length' applies here too...

        Michael Stack wrote:

        Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages.

        Devaraj Das wrote:

        I meant the argument on the PB encoding..

        The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId.

        Michael Stack wrote:

        Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.

        Devaraj Das wrote:

        Okay let's discuss that in a separate jira..

        Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated.

        Michael Stack wrote:

        There items above you said you'd address such as removing Header from the request and response and cleaning up doc in the .proto file, right?

        Correct .. that's what I meant to include in the new patch.

        • Devaraj

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

        On 2012-03-30 23:29:32, Devaraj Das wrote:

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

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

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

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

        (Updated 2012-03-30 23:29:32)

        Review request for Michael Stack and Benoit Sigoure.

        Summary

        -------

        Switch RPC call envelope/headers to PBs

        This addresses bug HBASE-5451.

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

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing

        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-02 00:21:20, Michael Stack wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto , line 93 > < https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93 > > > Should this precede the response? So if false, a response follows else an exception? Do we need a length here? Where is the header that the message name refers too? Devaraj Das wrote: Length will be taken care of by the protobuf serialization/deserialization. The header is the combination of callId, error. If the 'header' is confusing, I can take it off the object name. Let me know. Michael Stack wrote: Yeah, take away the header. Length I think is good. Makes it more robust (IIRC, we went out of our way to add length to the old RPC to help clients figure how much to pull). Devaraj Das wrote: The argument above for 'length' applies here too... Michael Stack wrote: Agreed. So high level, the response and request have a length of the total message? If so, don't need it down inside preceeding pb messages. Devaraj Das wrote: I meant the argument on the PB encoding.. The RPC response envelope, even today, doesn't include the length. For instance, the client side of the method HBaseClient.receiveResponse starts with reading the callId. Michael Stack wrote: Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix. Devaraj Das wrote: Okay let's discuss that in a separate jira.. Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated. Michael Stack wrote: There items above you said you'd address such as removing Header from the request and response and cleaning up doc in the .proto file, right? Correct .. that's what I meant to include in the new patch. Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/#review6613 ----------------------------------------------------------- On 2012-03-30 23:29:32, Devaraj Das wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-03-30 23:29:32) Review request for Michael Stack and Benoit Sigoure. Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-04-03 00:54:26.369788)

        Review request for Michael Stack and Benoit Sigoure.

        Changes
        -------

        This addresses the last set of comments from Stack (to do with changing the message names and marking certain fields optional, documentation on RPC spec).

        Summary
        -------

        Switch RPC call envelope/headers to PBs

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

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION

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

        Testing
        -------

        Thanks,

        Devaraj

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4096/ ----------------------------------------------------------- (Updated 2012-04-03 00:54:26.369788) Review request for Michael Stack and Benoit Sigoure. Changes ------- This addresses the last set of comments from Stack (to do with changing the message names and marking certain fields optional, documentation on RPC spec). Summary ------- Switch RPC call envelope/headers to PBs This addresses bug HBASE-5451 . https://issues.apache.org/jira/browse/HBASE-5451 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644 http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION Diff: https://reviews.apache.org/r/4096/diff Testing ------- Thanks, Devaraj
        Hide
        stack added a comment -

        Version of DD's patch that agrees w/ the just-about pattern we've established in src/main/protobuf. Renamed the .proto file RPC.proto from RPCMessageProtos and have it generate a wrapper calss RPCProtos rather than RPCMessageProtos and then each inner class has the Proto stripped. Did an edit on the .proto file comment to make class names agree w/ what is now generated. Seems to work. Going to pass via hadoopqa.

        Show
        stack added a comment - Version of DD's patch that agrees w/ the just-about pattern we've established in src/main/protobuf. Renamed the .proto file RPC.proto from RPCMessageProtos and have it generate a wrapper calss RPCProtos rather than RPCMessageProtos and then each inner class has the Proto stripped. Did an edit on the .proto file comment to make class names agree w/ what is now generated. Seems to work. Going to pass via hadoopqa.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521102/5305v7.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 does not introduce any 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.TestMultithreadedTableMapper
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapreduce.TestTableMapReduce

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//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/12521102/5305v7.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 does not introduce any 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.TestMultithreadedTableMapper org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapreduce.TestTableMapReduce Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1375//console This message is automatically generated.
        Hide
        stack added a comment -

        I ran the non-usual test fails locally and they pass. Will commit tomorrow unless objection.

        Show
        stack added a comment - I ran the non-usual test fails locally and they pass. Will commit tomorrow unless objection.
        Hide
        stack added a comment -

        Retry

        Show
        stack added a comment - Retry
        Hide
        Devaraj Das added a comment -

        Thank you, Stack!

        Show
        Devaraj Das added a comment - Thank you, Stack!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521110/5305v7.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 does not introduce any 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.TestMultithreadedTableMapper
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapreduce.TestTableMapReduce

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//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/12521110/5305v7.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 does not introduce any 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.TestMultithreadedTableMapper org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapreduce.TestTableMapReduce Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1376//console This message is automatically generated.
        Hide
        stack added a comment -

        Ran the failed tests locally and they pass. Committed trunk. Thanks for the patch Devaraj.

        Show
        stack added a comment - Ran the failed tests locally and they pass. Committed trunk. Thanks for the patch Devaraj.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2706 (See https://builds.apache.org/job/HBase-TRUNK/2706/)
        HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1309019)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        • /hbase/trunk/src/main/protobuf/RPC.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2706 (See https://builds.apache.org/job/HBase-TRUNK/2706/ ) HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1309019) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java /hbase/trunk/src/main/protobuf/RPC.proto
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #157 (See https://builds.apache.org/job/HBase-TRUNK-security/157/)
        HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1309019)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java
        • /hbase/trunk/src/main/protobuf/RPC.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #157 (See https://builds.apache.org/job/HBase-TRUNK-security/157/ ) HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1309019) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java /hbase/trunk/src/main/protobuf/RPC.proto
        Hide
        Gary Helmling added a comment -

        This change completely breaks SecureRpcEngine, which extended HBaseClient, HBaseServer, ConnectionHeader to share common functionality. I think any changes to the RPC layer should be tested with "-P security" as well as without.

        The org.apache.hadoop.hbase.security.User.createUser() addition also leaks out the previous User encapsulation of secure vs. non-secure UGI usage. It seemed the majority was in favor of requiring Hadoop 1.0+ for HBase 0.96 in the dev list discussion, so I'm not sure this is a major issue. But seems like it would be better for consistency to push the UGI calls down in to User.SecureHadoopUser. Or we could discuss removing User.HadoopUser as cleanup in 0.96 if it's no longer supported.

        Is there a plan with how the move the PB's integrates with security?

        Show
        Gary Helmling added a comment - This change completely breaks SecureRpcEngine, which extended HBaseClient, HBaseServer, ConnectionHeader to share common functionality. I think any changes to the RPC layer should be tested with "-P security" as well as without. The org.apache.hadoop.hbase.security.User.createUser() addition also leaks out the previous User encapsulation of secure vs. non-secure UGI usage. It seemed the majority was in favor of requiring Hadoop 1.0+ for HBase 0.96 in the dev list discussion, so I'm not sure this is a major issue. But seems like it would be better for consistency to push the UGI calls down in to User.SecureHadoopUser. Or we could discuss removing User.HadoopUser as cleanup in 0.96 if it's no longer supported. Is there a plan with how the move the PB's integrates with security?
        Hide
        stack added a comment -

        Is there a plan with how the move the PB's integrates with security?

        No other than we don't want to break it (we should have been more proactive around security changes G).

        Show
        stack added a comment - Is there a plan with how the move the PB's integrates with security? No other than we don't want to break it (we should have been more proactive around security changes G).
        Hide
        Andrew Purtell added a comment - - edited

        This patch should be reverted until the changes are refactored for sharing between the RPC engines.

        Edit: Subclassing of RPC engines was introduced to deal with incompatibilities in security related classes between Hadoop versions. If we are specifying Hadoop 1.0 (or equivalent shims) as a build requirement, then we could instead use a single RPC envelope and the User abstraction to paper over the remaining differences.

        Show
        Andrew Purtell added a comment - - edited This patch should be reverted until the changes are refactored for sharing between the RPC engines. Edit: Subclassing of RPC engines was introduced to deal with incompatibilities in security related classes between Hadoop versions. If we are specifying Hadoop 1.0 (or equivalent shims) as a build requirement, then we could instead use a single RPC envelope and the User abstraction to paper over the remaining differences.
        Hide
        Devaraj Das added a comment -

        For the record, the discussion & resolution of the build issue moved to HBASE-5727.

        Show
        Devaraj Das added a comment - For the record, the discussion & resolution of the build issue moved to HBASE-5727 .
        Hide
        stack added a comment -

        This patch should be reverted until the changes are refactored for sharing between the RPC engines.

        Is this still the case? An alternative is apply hbase-5727 to fix the broke build and then unify the rpcs?

        Show
        stack added a comment - This patch should be reverted until the changes are refactored for sharing between the RPC engines. Is this still the case? An alternative is apply hbase-5727 to fix the broke build and then unify the rpcs?
        Hide
        Andrew Purtell added a comment -

        We agreed a simple fix for the build in HBASE-5727 is fine as an interim step.

        Show
        Andrew Purtell added a comment - We agreed a simple fix for the build in HBASE-5727 is fine as an interim step.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #158 (See https://builds.apache.org/job/HBase-TRUNK-security/158/)
        HBASE-5727 secure hbase build broke because of 'HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1310073)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #158 (See https://builds.apache.org/job/HBase-TRUNK-security/158/ ) HBASE-5727 secure hbase build broke because of ' HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1310073) Result = FAILURE stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2715 (See https://builds.apache.org/job/HBase-TRUNK/2715/)
        HBASE-5727 secure hbase build broke because of 'HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1310073)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2715 (See https://builds.apache.org/job/HBase-TRUNK/2715/ ) HBASE-5727 secure hbase build broke because of ' HBASE-5451 Switch RPC call envelope/headers to PBs (Revision 1310073) Result = FAILURE stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2731 (See https://builds.apache.org/job/HBase-TRUNK/2731/)
        HBASE-5727 secure hbase build broke because of 'HBASE-5451 Switch RPC call envelope/headers to PBs' (Revision 1311287)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2731 (See https://builds.apache.org/job/HBase-TRUNK/2731/ ) HBASE-5727 secure hbase build broke because of ' HBASE-5451 Switch RPC call envelope/headers to PBs' (Revision 1311287) Result = SUCCESS stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #164 (See https://builds.apache.org/job/HBase-TRUNK-security/164/)
        HBASE-5727 secure hbase build broke because of 'HBASE-5451 Switch RPC call envelope/headers to PBs' (Revision 1311287)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #164 (See https://builds.apache.org/job/HBase-TRUNK-security/164/ ) HBASE-5727 secure hbase build broke because of ' HBASE-5451 Switch RPC call envelope/headers to PBs' (Revision 1311287) Result = FAILURE stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            Devaraj Das
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development