flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] curcur edited a comment on pull request #16606: [FLINK-21357][runtime/statebackend]Periodic materialization for generalized incremental checkpoints
Date Tue, 14 Sep 2021 09:00:50 GMT

curcur edited a comment on pull request #16606:
URL: https://github.com/apache/flink/pull/16606#issuecomment-918955439


   > Thanks a lot for the write-up Yuan!
   > 
   > To clarify my concerns regarding option 2:
   > States (e.g. `ChangelogValueState`) [connect](https://github.com/apache/flink/blob/7901f26d3470389c7fcfc720e2bab32cc226f1e6/flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/ChangelogValueState.java#L62)
`StateChangelogWriter` with the underlying backend: each state change must be reflected atomically
in both.
   > When creating a snapshot, mirroring this atomicity is crucial for correctness.
   > That's why I think it's better to keep these two parts as close to each other as possible
(and states are part of the `ChangelogKeyedStateBackend`, while materializer is more independent).
   
   Yes, this part I agree. The problem of put `ChangelogSnapshotState` in `PeriodicMaterializer`
is that we have sync up in these two places to guarantee this atomicity
   
   
   > In option 3, circular dependency is redundant if `triggerMaterialization` method is
removed (that's what I did in the original commit: instead of triggering the materialization,
test can [waits](https://github.com/rkhachatryan/flink/commit/61f37810ecccca53791547a052d4e895b8e4f513#diff-8654aaea028ca18fe809d05ce46a4a17cf3a4b18f72ec588f02611d47956eb19R5140)
materiazation to happen). So it seems like this approach doesn't have major concerns.
   
   This part I do not agree with. It is not about tests. 
   As long as you have `ChangelogSnapshotState` and its update methods in `ChangelogKeyedStatebackend`,
you will have the same problem. Unless you also put the periodic triggering logic into `ChangelogKeyedStatebackend`,
I cannot see how you can avoid circular constructor. But if you do so, you purely leave uploading
logic in `PeriodicMaterializationManager`.
   
   Then, it is very similar to Option1.
   
   


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message