reswqa commented on code in PR #22084:
URL: https://github.com/apache/flink/pull/22084#discussion_r1129012456
##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -503,12 +508,11 @@ private void onGlobalPoolAvailable() {
mayNotifyAvailable(toNotify);
}
+ @GuardedBy("availableMemorySegments")
private boolean shouldBeAvailable() {
assert Thread.holdsLock(availableMemorySegments);
- return !availableMemorySegments.isEmpty()
- && unavailableSubpartitionsCount == 0
- && numberOfRequestedOverdraftMemorySegments == 0;
+ return !availableMemorySegments.isEmpty() &&
unavailableSubpartitionsCount == 0;
Review Comment:
> Before we allows convert numberOfRequestedOverdraftMemorySegments to
numberOfRequestedMemorySegments , there still be a situation where both
availableMemorySegment and overdraft buffer are all not zero.
Just as what I said, only after we supports convert
`numberOfRequestedOverdraftMemorySegments` to
`numberOfRequestedMemorySegments`, then can we avoid having both `available
buffers` and `overdraft buffers`.
It is not enough to just modify the following code as there are still
concurrency problems.
```
if (!availableMemorySegments.isEmpty()) {
segment = availableMemorySegments.poll();
} else if (isRequestedSizeReached()) {
// Only when the buffer request reaches the upper limit(i.e.
current pool size),
// requests an overdraft buffer.
segment = requestOverdraftMemorySegmentFromGlobal();
}
```
Of course, we have allowed this conversion now, but I'm not very sure
whether there are other concurrency problems even though after this fix.
In any case, your reason is also make sense and acceptable to me. But for
the sake of safety, perhaps we should listen to the opinions of @wsry who knows
more about the implementation of `LocalBufferPool` than me.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
