zhijiangW commented on a change in pull request #7186: [FLINK-10941] Keep slots 
which contain unconsumed result partitions
URL: https://github.com/apache/flink/pull/7186#discussion_r238983295
 
 

 ##########
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/PartitionRequestQueue.java
 ##########
 @@ -134,10 +138,17 @@ public void cancel(InputChannelID receiverId) {
                ctx.pipeline().fireUserEventTriggered(receiverId);
        }
 
-       public void close() {
+       public void close() throws IOException {
                if (ctx != null) {
                        ctx.channel().close();
                }
+
+               LOG.info("Close all {} readers pending for close.", 
readersToClose.size());
 
 Review comment:
   Sorry for late response because of a little busy these days.
   
   I agree with your above comments. So whether the task executor can be 
released is based on whether there are active channels in this executor. The 
task executor can only exit after all the tcp connections are closed gracefully.
   
   In theory as long as the downstream received all the data from the network, 
then the upstream side can be released normally, no need to wait all the 
received data are processed completely by downstream side. But we have on 
existing ack mechanism to notify upstream side of this, so it is easy to rely 
on close request currently.  Based on downstream's consumption to release 
upstream's resource, it may get extra benefits in failover scenarios in future 
for persistent output files in upstream side, because the upstream do not need 
to restart during consumption exception in downstream side.
   
   But I just a little wonder it might bring potential effects in future via 
close request. For example, if there are 10 downstream tasks reuse the same tcp 
connection, and 9 tasks are finished earlier and only one tail task delay long 
to finish, then all the 10 partition views must be released together until the 
last downstream task finished. Although it might be no bad effects for delay 
releasing partition views currently.
   
   I would continue reviewing other parts of this PR and it may take some days 
on my side. :)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to