[
https://issues.apache.org/jira/browse/HBASE-3939?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13143591#comment-13143591
]
Gary Helmling commented on HBASE-3939:
--------------------------------------
Would be easier to review if the latest patch was in review board.
Here are comments from what I see:
In {{HBaseServer}}:
# In {{setupResponse()}} we take a {{Status}} instance (which is good), but it
doesn't get passed back through to the client. If we're modifying the wire
protocol to pass RPC version, I think we should also take advantage of the
change to serialize {{Status}} back to the client. This allows the client to
differentiate between fatal errors (authentication error, or bad rpc version)
which should kill the connection, and request-specific errors. By the way,
adding {{Status}} here allows me to remove it from the HBASE-2742 patch, which
is great!
In {{Invocation}}:
# In the constructor:
{code}
rpcVersion = WritableRpcEngine.writableRpcVersion;
{code}
This binds {{Invocation}} unnecessarily to {{WritableRpcEngine}}. We've
diverged a little from Hadoop RPC by making {{Invocation}} a top-level class,
allowing it to be shared between RPC engine implementations, so this would
undermine that. Since {{rpcVersion}} only seems to relate to {{Invocation}}
serialization, why not just define {{RPC_VERSION}} as a static final constant
on {{Invocation}}? Or alternately, couldn't we just make {{Invocation}}
implement {{VersionedWritable}} and let that handle the check for us?
Other than that, this patch looks fine to me. I've applied it together with
the HBASE-2742 patch and run through a few of the tests, and so far seems to
work fine.
> Some crossports of Hadoop IPC fixes
> -----------------------------------
>
> Key: HBASE-3939
> URL: https://issues.apache.org/jira/browse/HBASE-3939
> Project: HBase
> Issue Type: Bug
> Affects Versions: 0.92.0
> Reporter: Todd Lipcon
> Priority: Critical
> Fix For: 0.92.0
>
> Attachments: 3939-v2.txt, 3939-v3.txt, 3939-v4.txt, 3939-v5.txt,
> 3939-v6.txt, 3939-v7.txt, 3939.txt
>
>
> A few fixes from Hadoop IPC that we should probably cross-port into our copy:
> - HADOOP-7227: remove the protocol version check at call time
> - HADOOP-7146: fix a socket leak in server
> - HADOOP-7121: fix behavior when response serialization throws an exception
> - HADOOP-7346: send back nicer error response when client is using an out of
> date IPC version
--
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
