Skip to content

Conversation

@alexbegg
Copy link
Contributor

@alexbegg alexbegg commented Aug 28, 2020

Closes: #10350
Replaces PR #10556

It has been reported that the last run's execution date was incorrectly being displayed for both the last run's link text as well as the "Start Date" in the (i) tooltip on the DAGs page. This was happening in the RBAC UI. For example, this was wrong, the "Start Date" was not 2020-08-08T00:00:00+00:00, that is the execution_date:
Screen Shot 2020-08-28 at 11 42 09 AM

This PR fixes it so it shows the actual start_date for the "Start Date" tooltip instead of the execution_date, like it used to do in non-RBAC (Flask-admin) UI and before the RBAC UI was changed to load the latest_dagruns asynchronously:
Screen Shot 2020-08-28 at 11 41 00 AM

Ever since the PR #4005 which loads the latest_dagruns asynchronously the correct Start Date was lost. This PR replaces the correct Start Date.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Aug 28, 2020
Copy link
Member

@ryw ryw left a comment

Choose a reason for hiding this comment

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

LGTM, and I believe this is the right direction. I'd say we merge this in, then @alexbegg you can add next_run in a second PR?

@ryw ryw requested a review from kaxil August 31, 2020 15:03
@alexbegg
Copy link
Contributor Author

@ryw yeah, I'll make a second PR to add a next_run date

@JeffryMAC
Copy link

Great! BTW tooltip seems not to work on all browsers #10279

Copy link
Member

Choose a reason for hiding this comment

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

at this point this function name needs changing too

Copy link
Contributor Author

@alexbegg alexbegg Sep 2, 2020

Choose a reason for hiding this comment

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

Well, I am not adding new functionality, just fixing a bug/regression. I am just putting back a value lost when the values were changed to load asynchronously. But it is a valid point as that function was added at that point it was changed to be asynchronous.

This function appears to be named the same as the /last_dagruns endpoint that returns json for these dates, which is in turn seems to be named after the "Last Run" column this tooltip is in. So the column is showing the last run's execution date and the last run's start date. What name should it be instead?

Copy link
Contributor Author

@alexbegg alexbegg Sep 2, 2020

Choose a reason for hiding this comment

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

Correction, the function is named after the dag.get_last_dagrun method which was used in the pre-asynchronous code for the value of last_run, which then was used for the values last_run.execution_date and last_run.start_date, but was incorrectly changed to be last_run = execution_date when it went asynchronous in commit 6607e48#diff-f38558559ea1b4c30ddf132b7f223cf9L122. This PR just puts the values for the asynchronous code back to as they were.

@alexbegg
Copy link
Contributor Author

alexbegg commented Sep 2, 2020

Great! BTW tooltip seems not to work on all browsers #10279

@JeffryMAC thanks for the heads up, I'll try to look into that

Comment on lines +566 to +731
Copy link
Member

Choose a reason for hiding this comment

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

This could be incorrect an some (unlikely) circumstance:

If I create an dag run for an older execution_date, this would show mixed values.

Unfortunately doing this correctly in a single SQL query is harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can test for that, thanks for the heads up. However I would think the value above it, the execution_date, would be wrong to get the max value in the query, not the run_date. I don't recall that being the case as that query has been the same for a long time now (I just renamed the column alais) but if it is wrong, I'll try and fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, got busy with other stuff, I'll look into this soon

Copy link
Member

Choose a reason for hiding this comment

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

Fair point -- if you could rebase this PR we can merge this as is, as this is right in most cases, and better than the current broken behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebased now

@alexbegg alexbegg force-pushed the fix-last-run-start-date branch from f399294 to 0e24bf4 Compare October 19, 2020 22:25
@ashb ashb merged commit cd0ed7d into apache:master Oct 20, 2020
@JeffryMAC
Copy link

@kaxil @ashb can this be in next 1.10 ? it's a bug fix to the UI

@kaxil kaxil added this to the Airflow 1.10.13 milestone Oct 29, 2020
@kaxil
Copy link
Member

kaxil commented Oct 29, 2020

@kaxil @ashb can this be in next 1.10 ? it's a bug fix to the UI

Sure added it for 1.10.13 milestone

kaxil pushed a commit that referenced this pull request Nov 19, 2020
kaxil pushed a commit that referenced this pull request Nov 20, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
@alexbegg alexbegg deleted the fix-last-run-start-date branch August 19, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start Date in main DAG view shows redundant information

5 participants