----------------------------------------------------------- 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 On 2010-06-17 16:33:02, Philip Zeyliger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/210/ > ----------------------------------------------------------- > > (Updated 2010-06-17 16:33:02) > > > Review request for Avro. > > > Summary > ------- > > This is Patrick Wendell's patch from > https://issues.apache.org/jira/browse/AVRO-578 > > > Diffs > ----- > > trunk/lang/java/src/java/org/apache/avro/ipc/RPCContext.java 955405 > trunk/lang/java/src/java/org/apache/avro/ipc/Requestor.java 955405 > trunk/lang/java/src/java/org/apache/avro/ipc/Responder.java 955405 > trunk/lang/java/src/java/org/apache/avro/ipc/stats/StatsPlugin.java 955405 > trunk/lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java 955405 > trunk/lang/java/src/test/java/org/apache/avro/RPCMetaTestPlugin.java 955405 > > trunk/lang/java/src/test/java/org/apache/avro/ipc/stats/TestStatsPluginAndServlet.java > 955405 > > Diff: http://review.hbase.org/r/210/diff > > > Testing > ------- > > > Thanks, > > Philip > >
