[GitHub] [flink] akalash commented on a diff in pull request #22381: [FLINK-31763][runtime] Convert requested buffers to overdraft buffers when pool size is decreased

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

akalash commented on code in PR #22381:
URL: https://github.com/apache/flink/pull/22381#discussion_r1166841760


##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -766,13 +776,12 @@ private void returnExcessMemorySegments() {
 
     @GuardedBy("availableMemorySegments")
     private boolean hasExcessBuffers() {
-        return numberOfRequestedOverdraftMemorySegments > 0
-                || numberOfRequestedMemorySegments > currentPoolSize;
+        return numberOfRequestedOverdraftMemorySegments > 0;

Review Comment:
   Technically, this change is correct but since `numberOfRequestedOverdraftMemorySegments` and `numberOfRequestedMemorySegments` can be changed independently I would prefer to leave it as is. And we can make this change as soon as we get rid of `numberOfRequestedOverdraftMemorySegments`(if we will do it at all).
   Maybe I am a little paranoic about this but the current contract between `numberOfRequestedOverdraftMemorySegments` and `numberOfRequestedMemorySegments` looks a little bit fragile so I am afraid that it is easy to introduce the bug when `numberOfRequestedOverdraftMemorySegments > 0 == false` while `umberOfRequestedMemorySegments > currentPoolSize == true`



##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java:
##########
@@ -766,13 +776,12 @@ private void returnExcessMemorySegments() {
 
     @GuardedBy("availableMemorySegments")
     private boolean hasExcessBuffers() {
-        return numberOfRequestedOverdraftMemorySegments > 0
-                || numberOfRequestedMemorySegments > currentPoolSize;
+        return numberOfRequestedOverdraftMemorySegments > 0;
     }
 
     @GuardedBy("availableMemorySegments")
     private boolean isRequestedSizeReached() {
-        return numberOfRequestedMemorySegments >= currentPoolSize;
+        return numberOfRequestedMemorySegments == currentPoolSize;

Review Comment:
   I think you should not change this condition. Since, in my opinion, the rule of thumb for concurrent code looks something like this: "If the value physically can be greater than the border value we should also compare it with the border as >= even if logically it can not be greater". 
   In this case, we have the moment of time when `numberOfRequestedMemorySegments > currentPoolSize` it is under the synchronization but anyway.



-- 
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