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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
