[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Hudson (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Hudson (Commented) (JIRA)]

[ 
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

[Hudson (Commented) (JIRA)]

[ 
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

[Hudson (Commented) (JIRA)]

[ 
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

[Andrew Purtell (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Devaraj Das (Commented) (JIRA)]

[ 
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

[Andrew Purtell (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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

[Gary Helmling (Commented) (JIRA)]

[ 
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

[Hudson (Commented) (JIRA)]

[ 
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

[Hudson (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Hadoop QA (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Devaraj Das (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Hadoop QA (Commented) (JIRA)]

[ 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244882#comment-13244882
 ] 

jirapos...@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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244400#comment-13244400
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244389#comment-13244389
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244378#comment-13244378
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244377#comment-13244377
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244368#comment-13244368
 ] 

jirapos...@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.  > 
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.  > 
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.

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244365#comment-13244365
 ] 

jirapos...@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.  > 
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.  > 
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/HBaseC

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244362#comment-13244362
 ] 

jirapos...@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.  > 
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.  > 
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/hb

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244255#comment-13244255
 ] 

jirapos...@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.  > 
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.  > 
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   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.  > 
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.  > 
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.  > 
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.  > 
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 n

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244006#comment-13244006
 ] 

jirapos...@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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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 t

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13243919#comment-13243919
 ] 

jirapos...@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


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


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


We have an issue for removing this Invocation stuff?



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


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


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


We'll send this String each time?



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


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


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

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Hadoop QA (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13242848#comment-13242848
 ] 

jirapos...@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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241801#comment-13241801
 ] 

jirapos...@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.  > 
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 informatio

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241778#comment-13241778
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241769#comment-13241769
 ] 

jirapos...@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.  > 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13241464#comment-13241464
 ] 

jirapos...@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


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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13240722#comment-13240722
 ] 

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.  > 
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  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 Versio

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=1323#comment-1323
 ] 

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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239989#comment-13239989
 ] 

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/proto/RPCMessageProto.proto,
 line 71
bq.  > 
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.  > 
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.  > 
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. 

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
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.  > 
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.  > 
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.  > 
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.  > 
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/hb

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13238893#comment-13238893
 ] 

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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  Thank

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13237472#comment-13237472
 ] 

jirapos...@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


Trailing whitespaces.



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


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


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


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


Throw a ClassCastException.



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


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


Trailing whitespace.



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


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


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


just do return create(null) ?



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


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


Trailing whitespaces.



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


This seems unnecessary.



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


Kill all the trailing whitespaces!



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


I don't see how this is graceful.



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


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


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


Why is this optional?



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


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


Ditto, why have an extra PB?



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


This should

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[stack (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Devaraj Das (Commented) (JIRA)]

[ 
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

[Devaraj Das (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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

[Jimmy Xiang (Commented) (JIRA)]

[ 
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

[Devaraj Das (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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

[Todd Lipcon (Commented) (JIRA)]

[ 
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

[Devaraj Das (Commented) (JIRA)]

[ 
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  and 
 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

[Hadoop QA (Commented) (JIRA)]

[ 
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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220264#comment-13220264
 ] 

jirapos...@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


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

[Todd Lipcon (Commented) (JIRA)]

[ 
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

[Jimmy Xiang (Commented) (JIRA)]

[ 
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

[Devaraj Das (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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

[Hadoop QA (Commented) (JIRA)]

[ 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13219774#comment-13219774
 ] 

jirapos...@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

[Devaraj Das (Commented) (JIRA)]

[ 
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

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13218721#comment-13218721
 ] 

jirapos...@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] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[stack (Commented) (JIRA)]

[ 
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

[jira] [Commented] (HBASE-5451) Switch RPC call envelope/headers to PBs

[Devaraj Das (Commented) (JIRA)]

[ 
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

[stack (Commented) (JIRA)]

[ 
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