New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-21260] Add Finished state for DeclarativeScheduler #14912
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 0c9564d (Tue Feb 09 14:41:33 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating this PR @rmetzger. The changes look good to me. I had a comment concerning the tests. I think it would be a bit clearer if one sees the terminal state with which the ArchivedExecutionGraph
is initialized.
MockFinishedContext ctx = new MockFinishedContext(); | ||
Finished finished = createFinishedState(ctx); | ||
finished.onEnter(); | ||
|
||
assertThat(ctx.getArchivedExecutionGraph().getState(), is(JobStatus.FAILED)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at this test, it is not very clear to me why the archived execution graph is FAILED
. I assume that this hidden inside of createFinishedState
, right?
final ExecutionGraph executionGraph = TestingExecutionGraphBuilder.newBuilder().build(); | ||
executionGraph.failJob(new RuntimeException()); | ||
final ArchivedExecutionGraph archivedExecutionGraph = | ||
ArchivedExecutionGraph.createFrom(executionGraph); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could directly use the ArchivedExecutionGraphBuilder
. That way you don't have to make a detour creating an ExecutionGraph
.
Thank you for your feedback! I addressed the comments and will merge the change now! |
What is the purpose of the change
Declarative Scheduler consists of a number of internal states.
Note that this change is currently not usable as-is, as the other parts of declarative scheduler are not merged yet (See for the prototype this PR is based on: https://github.com/tillrohrmann/flink/tree/declarative-scheduler)
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation
Will be handled in a separate PR.