[ https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239925#comment-13239925 ]
jirapos...@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