otterc commented on a change in pull request #33034:
URL: https://github.com/apache/spark/pull/33034#discussion_r657623667
##########
File path:
common/network-common/src/main/java/org/apache/spark/network/client/TransportClient.java
##########
@@ -222,7 +223,7 @@ public void sendMergedBlockMetaReq(
handler.addRpcRequest(requestId, callback);
RpcChannelListener listener = new RpcChannelListener(requestId, callback);
channel.writeAndFlush(
- new MergedBlockMetaRequest(requestId, appId, shuffleId,
reduceId)).addListener(listener);
+ new MergedBlockMetaRequest(requestId, appId, shuffleId,
shuffleSequenceId, reduceId)).addListener(listener);
Review comment:
> mapId is similar to the shuffleSequenceId because even there we don't
fetch shuffle data for an older mapId mapped to the same mapIndex.
If the fetcher doesn't include a mapId in the request, how will the server
know which shuffle block to serve?
> I can't understand how introducing this parameter to the message is
suggestive that older shuffleSequenceId can also be fetched for a shuffle ID.
Semantics of the usage of a variable can always be clearly documented. I am not
convinced that adding this variable to the fetch protocol message is confusing,
if it is really confusing it can always be well documented.
If you are adding a field to a message that implies it has a specific need.
However, in this case there is no need to include that field. On the other
hand, tracking this information on the server is straightforward with minimal
memory usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
