[
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
