[GitHub] [flink] akalash commented on a diff in pull request #22084: [FLINK-31293][runtime] LocalBufferPool request overdraft buffer only when no available buffer and pool size is reached

["akalash (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org