[
https://issues.apache.org/jira/browse/FLINK-22376?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17340769#comment-17340769
]
Piotr Nowojski commented on FLINK-22376:
----------------------------------------
{quote}
P.S. I also don't see a lot of sense having the
BufferConsumer#CachedPositionMarker. So I would want to delete it.
{quote}
AFAIK it's required for correctness. For example a race condition between two
threads.
Thread 1:
{code}
bufferBuilder.appendAndCommit(...);
bufferBuilder.appendAndCommit(...);
bufferBuilder.finish();
{code}
Thread2:
{code}
bufferConsumer.build();
if (bufferConsumer.isFinished()) {
bufferConsumer.close();
}
{code}
Without separated positions, 2nd thread can build a buffer with just the data
from the first {{appendAndCommit}} call, but {{isFinished()}} would already
return true.
Secondly, it would cause more {{volatile}} reads.
About the recycling problem. I agree ideally it would be better for
{{BufferBuilder}} to do it's own reference counting. However I'm not sure if
that would be for free. That would have to be benchmarked carefully. So far we
were maintaining an implicit invariant, that {{BufferBuilder}} can not be
accessed after closing all of the consumers. It's not very clean but it was
working well. I'm not sure why did we have to add {{BufferBuilder#recycle}}
method.
> SequentialChannelStateReaderImpl may recycle buffer twice
> ---------------------------------------------------------
>
> Key: FLINK-22376
> URL: https://issues.apache.org/jira/browse/FLINK-22376
> Project: Flink
> Issue Type: Bug
> Components: Runtime / Network, Runtime / Task
> Affects Versions: 1.13.0
> Reporter: Roman Khachatryan
> Priority: Critical
> Fix For: 1.14.0, 1.13.1
>
>
> In ChannelStateChunkReader.readChunk in case of error buffer is recycled in
> the catch block. However, it might already have been recycled in
> stateHandler.recover().
> Using minor priority, as this only affects already failing path.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
