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.
I admit that we should not modify the current design for possible future
bugs. But there are some subtleties here : I somehow think that this is a
redundant judgment condition, and removing it can avoid bugs at the same time.
In any case, your reason is also make sense and acceptable to me. Therefore,
I will not strongly adhere to my opinion. 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
