hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs
Date Mon, 02 Apr 2012 06:42:29 GMT

    [ 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

        

Mime
View raw message