akalash commented on code in PR #22084:
URL: https://github.com/apache/flink/pull/22084#discussion_r1146481079
##########
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:
I actually mostly agree that `if (isRequestedSizeReached())` condition and
`while` inside `setNumBuffers` fix the bug of this ticket. It is difficult to
say something about removing `numberOfRequestedOverdraftMemorySegments == 0`.
On one hand, it is better doesn't change anything and improve it in another
PR(if we still think it makes sense) since it doesn't relate to this bug, on
the other hand, in the worst case(if we still have a bug) removing this
condition just allows us to allocate 'ordinary' buffer while we have already
overdraft buffers which is not so good but still better than have a deadlock.
Let me think about it a little more. But anyway if we will decide to remove it
I think it makes sense to do it in a separate commit just to understand where
is the fix and where is the prophylactic/improvement
In general, I would say that it seems we need to improve/refactor this class
a little(maybe I will create a ticket for that). Since I think we don't have a
strong/explicit relationship between
`numberOfRequestedOverdraftMemorySegments`, `numberOfRequestedMemorySegments`,
`availableMemorySegment` and `currentPoolSize` for example:
- `!availableMemorySegment.isEmpty() &&
numberOfRequestedOverdraftMemorySegments != 0` - never should be true
- `numberOfRequestedMemorySegments == currentPoolSize &&
availableMemorySegment.isEmpty()` - always should be true when
`numberOfRequestedOverdraftMemorySegments > 0`
- etc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
