zhijiangW commented on a change in pull request #7911: [FLINK-11082][network]
Fix the logic of getting backlog in sub partition
URL: https://github.com/apache/flink/pull/7911#discussion_r265845525
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/SpilledSubpartitionView.java
##########
@@ -147,7 +147,8 @@ public BufferAndBacklog getNextBuffer() throws
IOException, InterruptedException
return null;
}
- int newBacklog =
parent.decreaseBuffersInBacklog(current.isBuffer());
+ parent.decreaseBuffersInBacklog(current.isBuffer());
+ int newBacklog = parent.getBuffersInBacklog();
Review comment:
That is a good question that should be concerned. I considered it again and
thought the synchronized is not needed for getting backlog because this value
would be final consistent between sender and receiver.
E.g. if the current backlog is 4 after decreasing, the previous behavior
would report 4 strictly. The new behavior might report 4 or more than 4 if
increasing backlog again before getting. But the result is still correct if
reporting 5 because it actually exists. The difference is we report this
increase in advance, and the previous behavior would reflect this increase in
the next report. The early report might get some extra benefits because the
receiver could prepare more credits for it.
It could have two options:
1. Uniform all the ways of getting backlog outside of synchronized.
2. Integrate return backlog in `decreaseBuffersInBacklog` as you mentioned,
and the form seems not so bad if adding boolean parameter in
`decreaseBuffersInBacklog`. The backlog is strictly returned in this way.
Which option do you prefer?
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
