Skip to content
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

show correct duration on graph view for running task (#8311) #8675

Merged
merged 3 commits into from
Jun 27, 2020

Conversation

fuyi
Copy link
Contributor

@fuyi fuyi commented May 1, 2020

This PR is aim to fix issue #8311

Idea:

  1. Check if a task is still running.
  2. If so, disregard the duration field fetched from backend,
  3. calculate duration with current timestamp - ti.start_time

Tooltip before fix:

Screenshot 2020-05-01 at 22 11 23

Tooltip after fix:

screenshot1

The original bug of not showing duration at all on graph view is not the case on master branch, but, the value shows in the tooltip is not correct, since it is read from database and scheduler doesn't update duration frequently to reflect the actual duration till current timestamp for running tasks.

Note: The bug reported in issue 8311 is on v1.10.10 but this PR is based on master branch. Since there are fundamental change between master and v1.10.10 branch, separate PR might be needed to for v1.10.10.


Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@jeffolsi
Copy link

jeffolsi commented May 3, 2020

If I may point Invalid date is also misleading.
PR

There is nothing invalid here. It just didn't finish so there is no date in Ended.
It should be also modified to "still running" or "in progress" or something else that won't be considered as problem.

@fuyi
Copy link
Contributor Author

fuyi commented May 4, 2020

Hi @jeffolsi, Thanks for the comment. I agree with you that "invalid date" is not correct either. I have talked with @XD-DENG and leave this out of the scope of this PR.

@fuyi
Copy link
Contributor Author

fuyi commented May 29, 2020

Hi @ashb , Do you have any comment for this PR? : )

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Could you also fix/remove the Ended: invalid date please?

@fuyi
Copy link
Contributor Author

fuyi commented Jun 7, 2020

Hi @ashb, Ended: invalid date is now fixed

Screenshot 2020-06-07 at 15 28 01

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 (only a suggestion)

airflow/www/static/js/task-instances.js Show resolved Hide resolved
@fuyi
Copy link
Contributor Author

fuyi commented Jun 15, 2020

@ashb Do you have more comments? Can I get approval from you : )

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, and could you please update the screenshot in the PR?

airflow/www/static/js/task-instances.js Outdated Show resolved Hide resolved
@fuyi
Copy link
Contributor Author

fuyi commented Jun 27, 2020

Thanks for comment @ashb , The screenshot is updated now

@ashb ashb added this to the Airflow 1.10.12 milestone Jun 27, 2020
@ashb ashb merged commit 118ea2f into apache:master Jun 27, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 27, 2020

Awesome work, congrats on your first merged pull request!

kaxil pushed a commit to kaxil/airflow that referenced this pull request Jun 27, 2020
…pache#8675)

* show correct duration on graph view for running task (apache#8311)

* fix invalid end date  (apache#8311)

* Update airflow/www/static/js/task-instances.js

Co-authored-by: Ash Berlin-Taylor <[email protected]>

Co-authored-by: Ash Berlin-Taylor <[email protected]>
kaxil pushed a commit that referenced this pull request Aug 12, 2020
* fix invalid end date  (#8311)

* Update airflow/www/static/js/task-instances.js

Co-authored-by: Ash Berlin-Taylor <[email protected]>
kaxil pushed a commit that referenced this pull request Aug 15, 2020
* fix invalid end date  (#8311)

* Update airflow/www/static/js/task-instances.js

Co-authored-by: Ash Berlin-Taylor <[email protected]>
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
…pache#8675)

* fix invalid end date  (apache#8311)

* Update airflow/www/static/js/task-instances.js

Co-authored-by: Ash Berlin-Taylor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants