Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21346#discussion_r192825636
--- Diff:
common/network-common/src/main/java/org/apache/spark/network/server/RpcHandler.java
---
@@ -38,15 +38,28 @@
*
* This method will not be called in parallel for a single
TransportClient (i.e., channel).
*
+ * The rpc *might* included a data stream in <code>streamData</code>
(eg. for uploading a large
+ * amount of data which should not be buffered in memory here). An
error while reading data from
+ * the stream ({@link
org.apache.spark.network.client.StreamCallback#onData(String, ByteBuffer)})
+ * will fail the entire channel. A failure in "post-processing" the
stream in
+ * {@link
org.apache.spark.network.client.StreamCallback#onComplete(String)} will result
in an
+ * rpcFailure, but the channel will remain active.
+ *
+ * If streamData is not null, you *must* call
<code>streamData.registerStreamCallback</code>
+ * before this method returns.
+ *
* @param client A channel client which enables the handler to make
requests back to the sender
* of this RPC. This will always be the exact same object
for a particular channel.
* @param message The serialized bytes of the RPC.
+ * @param streamData StreamData if there is data which is meant to be
read via a StreamCallback;
--- End diff --
I'm wondering if a separate callback for these streams wouldn't be better.
It would at the very least avoid having to change all the existing handlers.
But it would also make it clearer what the contract is. For example, the
callback could return the stream callback to be registered.
It also doesn't seem like `StreamData` itself has a lot of useful
information other than the registration method, so it could be replaced with
parameters in the new callback, avoiding having to expose that type to RPC
handlers.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
