akalash commented on a change in pull request #15885:
URL: https://github.com/apache/flink/pull/15885#discussion_r634556819
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/BufferBuilder.java
##########
@@ -153,7 +153,12 @@ public BufferRecycler getRecycler() {
}
public void recycle() {
- recycler.recycle(memorySegment);
+ // If at least one consumer was created then they responsible for the
memory recycling
+ // because BufferBuilder doesn't contain a references counter so it
will be impossible to
+ // correctly recycle memory here.
+ if (!bufferConsumerCreated) {
+ recycler.recycle(memorySegment);
+ }
Review comment:
I have investigated the code a little more and now I understand that it
is not so easy to rewrite `BufferBuilder`(at least I see pretty strange usage
of `BufferBuilder` inside of `PartitionSortedBuffer` and
`SortMergeResultPartition`). So I can propose for this PR to remove the first
commit because it is too controversial and merge the second commit as is - it
is not the solution for all problems but it is resolving one of them. And I
also suggest creating a ticket where we can think about the correct usage of
`MemorySegment` and remove it from the places where it doesn't make sense in
order to keep the contract simple and consistent.
Also, if we think that this ticket is not so urgent, we can discuss
`MemorySegment` usage here rather than in a separate ticket.
WDYT? @pnowojski
--
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]
