[ 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 <https://reviews.apache.org/r/4096/#comment13673> Trailing whitespaces. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java <https://reviews.apache.org/r/4096/#comment13674> You already import RpcRequestWithHeaderProto, so just use "RpcRequestWithHeaderProto.Builder" here, drop the leading "RPCMessageProtos." http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java <https://reviews.apache.org/r/4096/#comment13675> Trailing whitespaces here and below. Kill them all. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java <https://reviews.apache.org/r/4096/#comment13677> If you cast it to RpcRequestProto, then why not check if param is an instance of RpcRequestProto? Also you're missing a space right before "param". http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java <https://reviews.apache.org/r/4096/#comment13678> Throw a ClassCastException. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java <https://reviews.apache.org/r/4096/#comment13679> Argh, no, don't change this! I got other HBase devs to promise to not change this as it makes backwards compatible clients impossibly complicated. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java <https://reviews.apache.org/r/4096/#comment13680> Trailing whitespace. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java <https://reviews.apache.org/r/4096/#comment13739> Is there a way to avoid code duplication and unify this method with the on in the HBaseClient class? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java <https://reviews.apache.org/r/4096/#comment13740> Why wrap this line and the next around? I think this fits on one line without exceeding 80 columns. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java <https://reviews.apache.org/r/4096/#comment13670> just do return create(null) ? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java <https://reviews.apache.org/r/4096/#comment13671> Why use the fully qualified names here? Also kill the trailing whitespace. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java <https://reviews.apache.org/r/4096/#comment13672> Trailing whitespaces. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java <https://reviews.apache.org/r/4096/#comment13741> This seems unnecessary. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13742> Kill all the trailing whitespaces! http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13743> I don't see how this is graceful. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13744> Why keep this oddity of Hadoop RPC? Either rely on TCP keepalive, or add a Ping method to the RPC interface. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13745> What's the point of this message? Why not just put the callId in RpcRequestProto and be done with it? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13746> Why is this optional? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13747> Why is this optional? It should be required and it should be first. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13748> Ditto, why have an extra PB? http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13749> This should be first and it should be required. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto <https://reviews.apache.org/r/4096/#comment13750> This should also be required. - Benoit On 2012-03-01 03:40:14, Devaraj Das wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/4096/ bq. ----------------------------------------------------------- bq. bq. (Updated 2012-03-01 03:40:14) bq. bq. bq. Review request for . bq. bq. bq. Summary bq. ------- bq. bq. Switch RPC call envelope/headers to PBs bq. bq. bq. This addresses bug HBASE-5451. bq. https://issues.apache.org/jira/browse/HBASE-5451 bq. bq. bq. Diffs bq. ----- bq. bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1294899 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/DataOutputOutputStream.java 1294899 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1294899 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1294899 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1294899 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/proto/RPCMessageProto.proto PRE-CREATION bq. bq. Diff: https://reviews.apache.org/r/4096/diff bq. bq. bq. Testing bq. ------- bq. bq. bq. Thanks, bq. bq. Devaraj bq. bq. > Switch RPC call envelope/headers to PBs > --------------------------------------- > > Key: HBASE-5451 > URL: https://issues.apache.org/jira/browse/HBASE-5451 > Project: HBase > Issue Type: Sub-task > Components: ipc, master, migration, regionserver > Affects Versions: 0.94.0 > Reporter: Todd Lipcon > Assignee: Devaraj Das > Fix For: 0.96.0 > > Attachments: rpc-proto.2.txt, rpc-proto.3.txt, rpc-proto.patch.1_2 > > -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira