pnowojski commented on a change in pull request #9993:
[FLINK-14498][runtime]Introduce NetworkBufferPool#isAvailable() for interacting
with LocalBufferPool.
URL: https://github.com/apache/flink/pull/9993#discussion_r341200691
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPool.java
##########
@@ -269,6 +267,17 @@ private MemorySegment requestMemorySegmentFromGlobal()
throws IOException {
if (segment != null) {
numberOfRequestedMemorySegments++;
return segment;
+ } else if (isBlocking) {
+ // if the future is completed before the
callback is registered,
+ // the request thread will wait 2s before
polling an available
+ // segment from the global pool, which is not a
big problem.
Review comment:
> The advantage of this implementation is that it can reduce useless wakeup
and avoid 2 second waiting if the future is completed immediately.
I think more importantly, it cleans the code a bit. The previous `if
(Thread.holdsLock...)` check inside the runnable action attached to
`isAvailable()` looked quite strange, which you correctly spotted, as it
required long multi line explanation in the comment.
```
// if the future is completed before the callback is registered,
// the request thread will wait 2s before polling an available
// segment from the global pool, which is not a big problem
```
Also rest of the code is relaying on `isAvailable().get()`, it makes sense
to use the same pattern here, instead of `lock.wait()`.
> Maybe we need do tow more things: one is to wrap the ExecutionException as
IOException or InterruptedException to avoid the change of method signature
(including the caller); the other is to complete the available future when
buffer pool is destroyed to unblocking the main thread.
Makes sense, I intentinoally skipped the exceptions but good that you caught
the destroying case - I missed that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
