gaoyunhaii commented on a change in pull request #14734:
URL: https://github.com/apache/flink/pull/14734#discussion_r570174748
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraph.java
##########
@@ -464,7 +459,13 @@ public void failJobDueToTaskFailure(
checkpointsCleaner,
new
ScheduledExecutorServiceAdapter(checkpointCoordinatorTimer),
SharedStateRegistry.DEFAULT_FACTORY,
- failureManager);
+ failureManager,
+ new CheckpointBriefCalculator(
+ getJobID(),
+ sourceAndAllVertices.f0,
+ sourceAndAllVertices.f1,
+ sourceAndAllVertices.f1),
+ new
ExecutionAttemptMappingProvider(sourceAndAllVertices.f1));
Review comment:
I also more like to move the creation to be inside
`CheckpointPlanCalculator`. However, since currently the unit tests relies on
passing mock vertices directly on creating `CheckpointCoordinator`, thus I
think we may postpone the change to the next PR, in the second PR we would
refactor the current tests to be based on real `ExecutionGraph` and then we
could move the creation into inside `CheckpointPlanCalculator`.
I'm a little concern for make `ExecutionAttemptMappingProvider` to be depend
on `CheckpointPlanCalculator` since logically I think the two components should
be for two separate purposes. Besides, we would also adjust the logic of the
`CheckpointPlanCalculator` in the next PR. Do you think it would be also ok to
separate the two components and make them all depends on ExecutionGraph~?
----------------------------------------------------------------
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:
us...@infra.apache.org
