-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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-19000] Forward initialization timestamp from Dispatcher to ExecGraph #13368
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 59deb5d (Thu Sep 10 06:47:04 UTC 2020) 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.
The JobDetailsHandler uses the CREATED
timestamp for denoting the start of the job; are we sticking with those semantics? (Same with JobResult for calculating runtime, and WebMonitorUtils)
When we adjust the UI we also have to think about a backwards-compatibility path, so that older jobs can be still be displayed in the HistoryServer.
Beyond that, some minor comments.
ArchivedExecutionGraph result = dispatcher.requestJob(jobGraph.getJobID(), TIMEOUT).get(); | ||
|
||
// ensure all statuses are set in the ExecutionGraph | ||
assertThat(result.getStatusTimestamp(JobStatus.INITIALIZING), greaterThan(0L)); |
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.
maybe also check that initializing <= created, to prevent the order from being messed up?
|
||
// ensure job is running | ||
CommonTestUtils.waitUntilCondition(() -> dispatcherGateway.requestJobStatus(jobGraph.getJobID(), TIMEOUT).get() == JobStatus.RUNNING, | ||
Deadline.fromNow(Duration.of(10, ChronoUnit.SECONDS)), 5L); |
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.
Deadline.fromNow(Duration.of(10, ChronoUnit.SECONDS)), 5L); | |
Deadline.fromNow(Duration.ofSeconds(10), 5L); |
dispatcherGateway.submitJob(jobGraph, TIMEOUT).get(); | ||
|
||
// ensure job is running | ||
CommonTestUtils.waitUntilCondition(() -> dispatcherGateway.requestJobStatus(jobGraph.getJobID(), TIMEOUT).get() == JobStatus.RUNNING, |
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'd move all arguments on a separate line.
0ff28b0
to
2283347
Compare
Thanks a lot for your review. I believe it makes sense to change the semantics and use the INITIALIZING state to calculate the job duration for the web frontend. But first, I'll way for more comments & results from the CI system. |
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.
Changes look fine to me.
Thanks a lot for the review. I confirmed the UI works. Merging ... |
2283347
to
e20229c
Compare
What is the purpose of the change
FLINK-16866 ("Make job submission non-blocking") introduced a new JobStatus INITIALIZING. However, the
ExecutionGraph.stateTimestamps
field does not contain the proper timestamp for the INITIALIZING state (it will only be non-zero while the job is INITIALIZING).This change forwards the timestamp from the Dispatcher to the ExecutionGraph.
Brief change log
Verifying this change
The change includes a new test.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation
Not necessary.