You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "Todd Lipcon (Created) (JIRA)" <ji...@apache.org> on 2012/02/22 07:52:49 UTC
[jira] [Created] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Switch RPC call envelope/headers to PBs
---------------------------------------
Key: HBASE-5451
URL: https://issues.apache.org/jira/browse/HBASE-5451
Project: HBase
Issue Type: Sub-task
Reporter: Todd Lipcon
Assignee: Devaraj Das
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Andrew Purtell (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247460#comment-13247460 ]
Andrew Purtell commented on HBASE-5451:
---------------------------------------
This patch should be reverted until the changes are refactored for sharing between the RPC engines.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244377#comment-13244377 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. The argument above for 'length' applies here too...
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. I meant the argument on the PB encoding..
bq.
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13215861#comment-13215861 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
Thanks Stack, for the quick but detailed review...
bq. if (!head.hasUserInfo()) return;
bq. .. Then you'd save an indent of the whole body of the method.
Makes sense
bq. 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
bq. Is ConnectionContext actually the headers? Should it be called ConnectionHeader?
Ok
bq. 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 ..
bq. 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.
bq. Fatten doc on the proto file I'd say. Its going to be our spec.
Ok
bq. 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..
bq. 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).
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245369#comment-13245369 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
Thank you, Stack!
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247421#comment-13247421 ]
stack commented on HBASE-5451:
------------------------------
bq. 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).
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241801#comment-13241801 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-29 18:13:02, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq. >
bq. > 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-.
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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?
bq.
bq. I'll upload a patch shortly that doesn't change the version number in the RPC... (we can revisit this issue later).
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13238893#comment-13238893 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 34
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34>
bq. >
bq. > 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.
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>
bq. >
bq. > 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?
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>
bq. >
bq. > 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.
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 25
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line25>
bq. >
bq. > 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Jimmy Xiang (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220249#comment-13220249 ]
Jimmy Xiang commented on HBASE-5451:
------------------------------------
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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239999#comment-13239999 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239925#comment-13239925 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 34
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line34>
bq. >
bq. > Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>
bq. >
bq. > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Similarly, on the response side.
bq.
How hard to leave it out DD and add later if we need it?
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>
bq. >
bq. > Why is this optional?
bq.
bq. Devaraj Das wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hadoop QA (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220195#comment-13220195 ]
Hadoop QA commented on HBASE-5451:
----------------------------------
-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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220242#comment-13220242 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
bq. 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..
bq. 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.
bq. 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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13219774#comment-13219774 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13236955#comment-13236955 ]
stack commented on HBASE-5451:
------------------------------
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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Status: Open (was: Patch Available)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13218721#comment-13218721 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Resolution: Fixed
Hadoop Flags: Incompatible change,Reviewed
Status: Resolved (was: Patch Available)
Ran the failed tests locally and they pass. Committed trunk. Thanks for the patch Devaraj.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Status: Open (was: Patch Available)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244882#comment-13244882 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13240722#comment-13240722 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Status: Patch Available (was: Open)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hadoop QA (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220265#comment-13220265 ]
Hadoop QA commented on HBASE-5451:
----------------------------------
-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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Attachment: rpc-proto.3.txt
Patch merged with current trunk.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244255#comment-13244255 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 548
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548>
bq. >
bq. > We have an issue for removing this Invocation stuff?
bq.
bq. Devaraj Das wrote:
bq. No not yet. But I'll create one to do with this issue once this patch is committed.
Thanks
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 25
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25>
bq. >
bq. > 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?
bq. >
bq. > What is 'failure compatibility'? We are telling the client to go away, nicely (smile).
bq. >
bq. > What you think we should replace hrpc0x0005 with?
bq. >
bq. > this -> these
bq.
bq. Devaraj Das wrote:
bq. 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.,
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 28
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28>
bq. >
bq. > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say?
bq. >
bq. > Would be nice to have illustration on how the back and forth work.
bq.
bq. Devaraj Das wrote:
bq. 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
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 55
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55>
bq. >
bq. > We'll send this String each time?
bq.
bq. Devaraj Das wrote:
bq. 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
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>
bq. >
bq. > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?
bq. >
bq. > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. '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.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13236933#comment-13236933 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
Could someone please look at this. Thanks!
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Gary Helmling (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247399#comment-13247399 ]
Gary Helmling commented on HBASE-5451:
--------------------------------------
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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hadoop QA (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245016#comment-13245016 ]
Hadoop QA commented on HBASE-5451:
----------------------------------
-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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13219728#comment-13219728 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
Updated reviewboard patch has most of the review comments taken care of i think.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221170#comment-13221170 ]
stack commented on HBASE-5451:
------------------------------
Client APIs should be the same but yeah, lets get up on pb before 0.96.0; a blocker as per DD.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Andrew Purtell (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247611#comment-13247611 ]
Andrew Purtell commented on HBASE-5451:
---------------------------------------
We agreed a simple fix for the build in HBASE-5727 is fine as an interim step.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Todd Lipcon (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221127#comment-13221127 ]
Todd Lipcon commented on HBASE-5451:
------------------------------------
Sounds reasonable, thanks Devaraj.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245022#comment-13245022 ]
stack commented on HBASE-5451:
------------------------------
I ran the non-usual test fails locally and they pass. Will commit tomorrow unless objection.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Attachment: 5305v7.txt
Retry
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221159#comment-13221159 ]
stack commented on HBASE-5451:
------------------------------
Thats a dumb question. Let me rephrase. Won't hbase be all pb natively by 0.96.0?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244006#comment-13244006 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > 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.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java, line 25
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97739#file97739line25>
bq. >
bq. > We should just be using the hadoop DOOS... looks like no diff (when I diff them). I'll make an issue to remove.
cool
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 446
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line446>
bq. >
bq. > Is this written up anywhere? That its hrpc, then version, then a length, then a protobuf?
bq. >
bq. > I see it in the proto definition. That'll do.
cool
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 548
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97740#file97740line548>
bq. >
bq. > 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.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 25
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line25>
bq. >
bq. > 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?
bq. >
bq. > What is 'failure compatibility'? We are telling the client to go away, nicely (smile).
bq. >
bq. > What you think we should replace hrpc0x0005 with?
bq. >
bq. > this -> these
Yeah, valid points. We can remove this version string and all in a follow up patch.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 28
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line28>
bq. >
bq. > How does RpcRequestWithHeaderProto relate to ConnectionHeaderProto? This text should say?
bq. >
bq. > 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.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 55
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line55>
bq. >
bq. > We'll send this String each time?
Actually, I could make this field 'optional' since this has a default value. Will do so.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>
bq. >
bq. > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?
bq. >
bq. > 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..
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221157#comment-13221157 ]
stack commented on HBASE-5451:
------------------------------
Can we have hbase go all-pb for hbase 0.96.0?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221122#comment-13221122 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
bq. 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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220264#comment-13220264 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249935#comment-13249935 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Status: Open (was: Patch Available)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239989#comment-13239989 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 71
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line71>
bq. >
bq. > What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Similarly, on the response side.
bq.
bq.
bq. Michael Stack wrote:
bq. 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).
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto, line 72
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86905#file86905line72>
bq. >
bq. > Why is this optional?
bq.
bq. Devaraj Das wrote:
bq. 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?
bq.
bq. Michael Stack wrote:
bq. Sure in general. What about the specific comment? Seems like its required?
Ok.. I'll make the critical fields "REQUIRED"
bq. On 2012-03-24 07:38:03, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Status: Open (was: Patch Available)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241464#comment-13241464 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Attachment: rpc-proto.patch.1_2
First pass on the patch. I still need to get the responses on protobuf.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hadoop QA (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245425#comment-13245425 ]
Hadoop QA commented on HBASE-5451:
----------------------------------
-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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Status: Patch Available (was: Open)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Attachment: rpc-proto.r5.txt
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244368#comment-13244368 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>
bq. >
bq. > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?
bq. >
bq. > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. '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..
bq.
bq. Michael Stack wrote:
bq. 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?
bq.
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional.
bq.
bq. 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?
bq.
bq. (Also note that the top level RPC request envelope has the length preceding the request data)
bq.
bq. Michael Stack wrote:
bq. If the top level rpc request envelope has the length, then I agree w/ you, its not needed as prefix on pb messages.
cool
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. The argument above for 'length' applies here too...
bq.
bq. Michael Stack wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244400#comment-13244400 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. The argument above for 'length' applies here too...
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. I meant the argument on the PB encoding..
bq.
bq. 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.
bq.
bq. Michael Stack wrote:
bq. Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.
bq.
bq. Devaraj Das wrote:
bq. Okay let's discuss that in a separate jira..
bq.
bq. Otherwise, do you think the patch is good to go? If so, I'll submit a new patch with some of the comments incorporated.
bq.
bq. Michael Stack wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221163#comment-13221163 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
bq. Won't hbase be all pb natively by 0.96.0?
That should be the goal, rather a blocker *smile*
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249932#comment-13249932 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245749#comment-13245749 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Attachment: rpc-proto.2.txt
Uploading patch so I can submit this to hudson.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241769#comment-13241769 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-29 18:13:02, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq. >
bq. > 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-.
bq. >
bq. > 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247593#comment-13247593 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
For the record, the discussion & resolution of the build issue moved to HBASE-5727.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13242848#comment-13242848 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13237472#comment-13237472 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241778#comment-13241778 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-03-29 18:13:02, Benoit Sigoure wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 102
bq. > <https://reviews.apache.org/r/4096/diff/2/?file=86903#file86903line102>
bq. >
bq. > 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.
bq. >
bq. > 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-.
bq. >
bq. > 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.
bq.
bq. Devaraj Das wrote:
bq. 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?
bq.
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-01 03:40:14)
bq.
bq.
bq. Review request for .
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13243919#comment-13243919 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247029#comment-13247029 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247722#comment-13247722 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220219#comment-13220219 ]
stack commented on HBASE-5451:
------------------------------
I wonder how Jimmy is doing stuff like the below:
{code}
+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;
{code}
... 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:
{code}
+ 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()));
{code}
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!
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244378#comment-13244378 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. The argument above for 'length' applies here too...
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. I meant the argument on the PB encoding..
bq.
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247600#comment-13247600 ]
stack commented on HBASE-5451:
------------------------------
bq. 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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Status: Patch Available (was: Open)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244389#comment-13244389 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. The argument above for 'length' applies here too...
bq.
bq. Michael Stack wrote:
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. I meant the argument on the PB encoding..
bq.
bq. 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.
bq.
bq. Michael Stack wrote:
bq. Ok. We are replicating what was there previous. Lets make new jira for doing things like a length prefix.
bq.
bq. Devaraj Das wrote:
bq. Okay let's discuss that in a separate jira..
bq.
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Jimmy Xiang (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13221166#comment-13221166 ]
Jimmy Xiang commented on HBASE-5451:
------------------------------------
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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hadoop QA (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13242891#comment-13242891 ]
Hadoop QA commented on HBASE-5451:
----------------------------------
-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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Devaraj Das updated HBASE-5451:
-------------------------------
Fix Version/s: 0.96.0
Affects Version/s: 0.94.0
Status: Patch Available (was: Open)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247667#comment-13247667 ]
Hudson commented on HBASE-5451:
-------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Todd Lipcon (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220250#comment-13220250 ]
Todd Lipcon commented on HBASE-5451:
------------------------------------
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?)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244362#comment-13244362 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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...
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>
bq. >
bq. > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?
bq. >
bq. > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. '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..
bq.
bq. Michael Stack wrote:
bq. 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?
bq.
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Issue Comment Edited] (HBASE-5451) Switch RPC call
envelope/headers to PBs
Posted by "Andrew Purtell (Issue Comment Edited) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13247460#comment-13247460 ]
Andrew Purtell edited comment on HBASE-5451 at 4/5/12 6:25 PM:
---------------------------------------------------------------
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.
was (Author: apurtell):
This patch should be reverted until the changes are refactored for sharing between the RPC engines.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Status: Patch Available (was: Open)
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244365#comment-13244365 ]
jiraposter@reviews.apache.org commented on HBASE-5451:
------------------------------------------------------
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 66
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line66>
bq. >
bq. > Which part in here is the 'header'? How does it relate to ConnectionHeaderProto?
bq. >
bq. > request can be an Invocation/Writable? Or a protobuf? Do we need a length in here?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. '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..
bq.
bq. Michael Stack wrote:
bq. 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?
bq.
bq. 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.
bq.
bq. Devaraj Das wrote:
bq. Yes, I'll take off the 'header' from the message name. I could make the 'bytes' field optional.
bq.
bq. 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?
bq.
bq. (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.
bq. On 2012-04-02 00:21:20, Michael Stack wrote:
bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto, line 93
bq. > <https://reviews.apache.org/r/4096/diff/3/?file=97744#file97744line93>
bq. >
bq. > 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?
bq.
bq. Devaraj Das wrote:
bq. 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.
bq.
bq. Michael Stack wrote:
bq. 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).
bq.
bq. Devaraj Das wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4096/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-30 23:29:32)
bq.
bq.
bq. Review request for Michael Stack and Benoit Sigoure.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Switch RPC call envelope/headers to PBs
bq.
bq.
bq. This addresses bug HBASE-5451.
bq. https://issues.apache.org/jira/browse/HBASE-5451
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCMessageProtos.java PRE-CREATION
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1307644
bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPCMessageProto.proto PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4096/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Devaraj
bq.
bq.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "Devaraj Das (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13224544#comment-13224544 ]
Devaraj Das commented on HBASE-5451:
------------------------------------
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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13215378#comment-13215378 ]
stack commented on HBASE-5451:
------------------------------
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?
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
stack updated HBASE-5451:
-------------------------
Attachment: 5305v7.txt
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.
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Affects Versions: 0.94.0
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Fix For: 0.96.0
>
> Attachments: 5305v7.txt, rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2, rpc-proto.r5.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to
PBs
Posted by "stack (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13215895#comment-13215895 ]
stack commented on HBASE-5451:
------------------------------
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
> Switch RPC call envelope/headers to PBs
> ---------------------------------------
>
> Key: HBASE-5451
> URL: https://issues.apache.org/jira/browse/HBASE-5451
> Project: HBase
> Issue Type: Sub-task
> Components: ipc, master, migration, regionserver
> Reporter: Todd Lipcon
> Assignee: Devaraj Das
> Attachments: rpc-proto.patch.1_2
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira