gaoyunhaii commented on pull request #16655: URL: https://github.com/apache/flink/pull/16655#issuecomment-899655460
Hi Stephan @StephanEwen very thanks for the review and suggestions! They are indeed very instructive to me~ 1. The initial thought of `isFinishedOnRestore` is to mark the task as "could be marked as finished after restored", but "wasDeployedAsFinished" seems indeed more smooth to me. 2. `operatorsFinished` is introduced due to we need to distinguish between the point that task has called `finishOperators()` and is waiting for the last checkpoint / savepoint and the point that task is transit to FINISHED on the JM side, thus I think we might not be able to directly use `isTaskFinised` for the first case ? 3. I also agree we could limit the scope of tests to test `VertexFinishedStateChecker` only to decouple the checker with the `CheckpointCoordinator`~ 4. Currently `PendingCheckpoint` indeed depends on `CheckpointPlan` to provide some meta information about the current checkpoint. It is very instructive to me to let the `CheckpointCoordinator` to directly access the `CheckpointPlan` instead of proxying by the `PendingCheckpoint`. In this way we could be able to separate the logic with the finished states and the reported states. I'll have a try with this direction~ I also agree with we could do the code improvement in a dedicated PR. I opened a new issue https://issues.apache.org/jira/browse/FLINK-23815 for this. For the current PR I also fixed the above minor issues and squashed the commits. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
