[GitHub] [spark] xuanyuanking commented on a change in pull request #27665: [SPARK-24355][Core][FOLLOWUP] Add flag for fetching chunks in async mode

[GitBox <gi...@apache.org>]

xuanyuanking commented on a change in pull request #27665: [SPARK-24355][Core][FOLLOWUP] Add flag for fetching chunks in async mode
URL: https://github.com/apache/spark/pull/27665#discussion_r383080838
 
 

 ##########
 File path: common/network-common/src/main/java/org/apache/spark/network/server/ChunkFetchRequestHandler.java
 ##########
 @@ -124,7 +127,13 @@ private ChannelFuture respond(
       final Channel channel,
       final Encodable result) throws InterruptedException {
     final SocketAddress remoteAddress = channel.remoteAddress();
-    return channel.writeAndFlush(result).await().addListener((ChannelFutureListener) future -> {
+    ChannelFuture channelFuture = null;
+    if (chunkFetchAsyncModeEnabled) {
+      channelFuture = channel.writeAndFlush(result);
 
 Review comment:
   Thanks for the comment @otterc!
   This PR aims to fix the performance regression brings by the sync mode fetching. As you mention, you find a problem with the stress test framework, I think the default mode should be the guarantee for common use cases.
   
   For the SASL requests timing out, maybe we need more context for your internal stress test framework and analyze the root cause. IMO, there should be other configs in spark.shuffle/netty could help, not only depends on the async/sync mode here.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org