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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
