carp84 commented on issue #9501: [FLINK-12697] [State Backends] Support on-disk state storage for spill-able heap backend URL: https://github.com/apache/flink/pull/9501#issuecomment-527878386 Thanks for the continuous efforts on review @tillrohrmann ! Before addressing detailed comments, let me answer the general ones first: 1. About thread safety of the implementation: the `testConcurrentSnapshots`, `testCopyOnWriteContracts` and `testSnapshotPruneValues` cases in `CopyOnWriteSkipListStateMapTest` are designed to cover the possible concurrency with asynchronous checkpoint. There might be one or two fields could be `volatile` but not, but checking the logic those are all nice to improve but won't affect the correctness of logic even if keep them as is. Will talk more when addressing the specific comments. 2. About the design on translation from node pointer to `ByteBuffer` and offset, yes it's on purpose for performance consideration. If we construct a "Node" object (for example) every time, the cost of constructing object is relatively big comparing to "address"-only operations (and note that those address operations cannot be saved for off-heap data structure even if we have an abstraction of Node object). What's more, we don't need all fields of a Node for each operation, such as locating which Node a given key belongs to, we only need to compare keys until finding the target, all other "fields" of a Node is not needed during the process.
---------------------------------------------------------------- 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
