[GitHub] [flink] rkhachatryan commented on pull request #13648: [FLINK-19632] Introduce a new ResultPartitionType for Approximate Local Recovery

[GitBox <gi...@apache.org>]

rkhachatryan commented on pull request #13648:
URL: https://github.com/apache/flink/pull/13648#issuecomment-713798578


   Thanks a lot for the in-depth analysis and for updating the PR!
   It answered my original question about thread-safety.
   
   But I still have two other concerns:
   1. Visilibity in normal case: none of the felds written in `releaseView` are `volatile`. So in normal case (`t1:release` then `t2:createReadView`) `t2` can see some inconsistent state. For example, `readView == null`, but `isPartialBufferCleanupRequired == false`. Right?
   Maybe call `releaseView()`  from `createReadView()` unconditionally?
   
   2. Overwites when release is slow: won't `t1` overwrite changes to `PipelinedSubpartition` made already by `t2`? For example, reset `sequenceNumber` after `t2` has sent some data?
   Maybe `PipelinedSubpartition.readerView` should be `AtomicReference` and then we can guard `PipelinedApproximateSubpartition.releaseView()` by CAS on it?


----------------------------------------------------------------
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:
users@infra.apache.org