-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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-19518] Show proper job duration for running jobs in web ui #13560
[FLINK-19518] Show proper job duration for running jobs in web ui #13560
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 6f5dffb (Thu Oct 08 07:23:24 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.
Thanks for the fix, @rmetzger . I just have a few minor things. Additionally, I did a manual test to check whether the job overview shows the duration. ✅
int numTotalTasks = 0; | ||
|
||
for (AccessExecutionJobVertex ejv : job.getVerticesTopologically()) { | ||
AccessExecutionVertex[] vertices = ejv.getTaskVertices(); |
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.
AccessExecutionVertex[] vertices = ejv.getTaskVertices(); | |
AccessExecutionVertex[] taskVertices = ejv.getTaskVertices(); |
Could we do this renaming to improve the readability since we're dealing with different types of vertices in this code segment?
AccessExecutionVertex[] vertices = ejv.getTaskVertices(); | ||
numTotalTasks += vertices.length; | ||
|
||
for (AccessExecutionVertex vertex : vertices) { |
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.
for (AccessExecutionVertex vertex : vertices) { | |
for (AccessExecutionVertex taskVertex : taskVertices) { |
Same here with the vertex
variable.
@@ -303,6 +303,55 @@ public void testCancel() throws Exception { | |||
BlockingInvokable.reset(); | |||
} | |||
|
|||
/** | |||
* See FLINK-19518. This test ensures that the /jobs/overview handler shows a duration != 0. | |||
* |
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 know it's a minor thing, but the extra line is not necessary here.
BlockingInvokable.latch.await(); | ||
|
||
final Duration testTimeout = Duration.ofMinutes(2); | ||
final LocalTime deadline = LocalTime.now().plus(testTimeout); |
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 deadline
variable is never used. Are we missing an assert
in this test or is this variable obsolete?
sender.setInvokableClass(BlockingInvokable.class); | ||
|
||
final JobGraph jobGraph = new JobGraph("Stoppable streaming test job", sender); | ||
final JobID jid = jobGraph.getJobID(); |
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.
jid
is never used and can be removed.
Thanks a lot for your review. I addressed your comments. |
I'll merge it once CI has passed. |
7ea62ee
to
5000a17
Compare
5000a17
to
b64dbe9
Compare
Ci failed. Rebased to latest master. |
CI has passed now. Merging. |
What is the purpose of the change
Caused by FLINK-16866, the web UI is showing the "Duration" of a job as 0 in the overview. Once you open the detail page of a job, you see the correct duration.
Brief change log
createDetailsForJob
toJobDetails
requestJob
instead ofrequestJobStatus
to get all the job details in DispatcherJobVerifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation
Bugfix