pnowojski 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_r265551809
##########
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:
Something is wrong here. The line above is synchronized
(`decreaseBuffersInBacklog` vs `decreaseBuffersInBacklogUnsafe`) and this
`getBuffersInBacklog` is not (note miss-leading lack of `Unsafe` suffix) .
Either:
1. the above one doesn't need to be synchronized
2. there is a bug and this one should be synchronized. In that case however,
it would be additional unnecessary synchronisation cost, so I think I would
prefer to integrate `getBuffersInBacklog` with `decreaseBuffersInBacklog` after
all.
3. This doesn't need to be synchronized because of some hacky assumption. In
that case either write big comment or probably better and/or safer approach
would be to integrate `getBuffersInBacklog` with `decreaseBuffersInBacklog`
after all.
----------------------------------------------------------------
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]
With regards,
Apache Git Services
