StephanEwen commented on issue #8693: [FLINK-8871] Support to cancel checkpoing via notification URL: https://github.com/apache/flink/pull/8693#issuecomment-542834147 @Myasuka Thank you for working on this feature. This will be a good addition to Flink. The general mechanism makes sense to me. I noticed a few issues in the code that would be great to address before merging. **(1)** The trigger of checkpoints on barrier needs also to check if the checkpoint is already aborted. Similar to the triggering on RPC. **(2)** I think it would be worth considering to refactor all checkpointing parts into a single place. Something like a `Checkpoints` class that captures all currently running and pending / aborted checkpoints. That would simplify a lot, and also make tests much easier, because tests would not need to deal with mocking the `StreamTask`. **(3)** The PR introduces more lock synchronization, while the remaining parts are going for the mailbox instead. At least the checkpoint lock should be avoided in favor of the mailbox executor. **(4)** We should probably rename `notifyCheckpointAbort(...)` to `notifyCheckpointAborted(...)` **(5)** The `notifyCheckpointAbort` method in the `CheckpointListener` interface should probably not be a default method. Implementers should generally be forced to think about what to do when checkpoint is aborted. I think given the abstractions right now, it would make sense for the `AbstractStreamOperator` to have a default implementation of that method that is empty.
---------------------------------------------------------------- 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
