[
https://issues.apache.org/jira/browse/AVRO-578?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12880003#action_12880003
]
Philip Zeyliger commented on AVRO-578:
--------------------------------------
I threw this up on reviewboard (pretty nifty, btw!). I don't think we've
configured reviewboard to paste in its messages in the JIRA, so I'm manually
adding them. The thing I'm most worried about is Plugins keeping a hold of the
RpcContext object for longer than they ought to, and inadvertently keeping the
requests and responses.
-- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/210/#review255
-----------------------------------------------------------
For testing, you should check out
src/test/java/org/apache/avro/RPCMetaTestPlugin.java. You can assert that the
payload fields are set, that the right number of bytes is being sent and
received, etc.
trunk/lang/java/src/java/org/apache/avro/ipc/RPCContext.java
<http://review.hbase.org/r/210/#comment1106>
The other things here are either private or protected; this probably
deserves to be too.
trunk/lang/java/src/java/org/apache/avro/ipc/RPCContext.java
<http://review.hbase.org/r/210/#comment1107>
if this is the only accessor you have, do you need setPayload at all, or
should you provide access to the entire payload? If you provide access to the
payload, it might make sense to mark it in the javadoc as "read only", since
plugins really shouldn't be editing the payload on the fly.
trunk/lang/java/src/java/org/apache/avro/ipc/RPCContext.java
<http://review.hbase.org/r/210/#comment1104>
[nit] I think the style here is "for (A a : as) {" (i.d., space both before
and after ":", but I could be wrong.
trunk/lang/java/src/java/org/apache/avro/ipc/Requestor.java
<http://review.hbase.org/r/210/#comment1105>
It seems a bit odd to use the same field for both request and response
payloads.
I'm also a bit concerned that the payload data might be large, and that
we're storing it in this RpcContext object for potentially a while. Is the
RpcContext object being stored for a while?
One way to do this might be to add plugin.clientReceiveResponse(context,
response) (i.e., give the response directly to the plugin). That breaks the
cleanliness of the API. Another way to do it would be to explain in the
javadoc for the plugin that the payload is available only during
clientReceiveResponse.
- Philip
> Add RPC Payload to RPCContext class
> -----------------------------------
>
> Key: AVRO-578
> URL: https://issues.apache.org/jira/browse/AVRO-578
> Project: Avro
> Issue Type: Sub-task
> Components: java
> Reporter: Patrick Wendell
> Assignee: Patrick Wendell
> Attachments: AVRO-578.patch, AVRO-578.patch.v2
>
>
> For stats/monitoring it is helpful to see how many bytes are encoded for a
> given RPC call. Right now Encoder's don't track how many payload bytes are
> actually written out when encoding is done.
> Ideally this bytesWritten() would be in Encoder interface, however not sure
> JSON plugin can track the number of characters actually written, so
> alternatively just could be added to BinaryEncoder, and stats plugin will
> only provide payload sizes when that encoder is used.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
