akalash commented on a change in pull request #15885:
URL: https://github.com/apache/flink/pull/15885#discussion_r644239697
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/BufferWritingResultPartition.java
##########
@@ -348,6 +348,7 @@ private void finishUnicastBufferBuilder(int
targetSubpartition) {
if (bufferBuilder != null) {
numBytesOut.inc(bufferBuilder.finish());
numBuffersOut.inc();
+ bufferBuilder.recycle();
Review comment:
at least here
org.apache.flink.runtime.checkpoint.channel.ChannelStateChunkReader#readChunk.
And for example here
org.apache.flink.runtime.checkpoint.channel.ResultSubpartitionRecoveredStateHandler#recover,
right now we create BufferConsumer and only then finishing the BufferBuilder
and it is possible to safely replace the finish by the close but I think more
correctly in this method to finish BufferBuilder first, then create
BufferConsumer and only then close BufferBuilder.
But in general, I believe it is possible to reduce these two methods to one
(it is not a big problem even for readChunk because BufferConsumer is not
created yet). But in this case, we will leave only one possible invariant -
commit all changes only when BufferConsumer is created already. So filling the
buffer then committing changes and only then creating BufferConsumer will be
impossible.
So for example this code won't be work:
```
try(BufferBuilder bb = requestBufferBuilder()) {
bb.append(..);
read(bb.createBufferConsumer());
}
```
it will need to be rewritten like this:
```
BufferBuilder bb = requestBufferBuilder();
bb.append(..);
BufferConsumer bc = bb.createBufferConsumer();
bb.close();
read(bc);
```
In conclusion, I think that logically it is two different methods(different
responsibilities), but for simplification, it is possible to have only one
method if you think it makes sense.
--
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]
