-
Notifications
You must be signed in to change notification settings - Fork 14.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
[AIRFLOW-3160] (Unrevert) Load latest_dagruns asynchronously #5339
[AIRFLOW-3160] (Unrevert) Load latest_dagruns asynchronously #5339
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5339 +/- ##
==========================================
+ Coverage 79.02% 79.02% +<.01%
==========================================
Files 481 481
Lines 30197 30212 +15
==========================================
+ Hits 23864 23876 +12
- Misses 6333 6336 +3
Continue to review full report at Codecov.
|
ef29de0
to
1cc5ef2
Compare
{% endif %} | ||
<div height="10" width="10" id='last-run-{{ dag.safe_dag_id }}' style="display: block;"> | ||
<a></a> | ||
<img class="loading-last-run" width="15" src="{{ url_for("static", filename="loading.gif") }}"> |
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.
This image is wider than the div it's in - does it look okay?
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.
It looks ok to me in my browser, I agree probably the settings can be cleaned up and this width is strange. This is copied from all of the other async loaded components.
last_run = json[safe_dag_id].last_run; | ||
g = d3.select('div#last-run-' + safe_dag_id) | ||
g.selectAll('a') | ||
.attr("href", "{{ url_for('Airflow.graph') }}?dag_id=" + dag_id + "&execution_date=" + last_run) |
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.
.attr("href", "{{ url_for('Airflow.graph') }}?dag_id=" + dag_id + "&execution_date=" + last_run) | |
.attr("href", "{{ url_for('Airflow.graph') }}?dag_id=" + encodeUriComponent(dag_id) + "&execution_date=" + last_run) |
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.
Can make this change, seems safe enough and good in general. Note not encoding dag id is a common pattern in the UI (encoding is never done) and probably needs to be factored into some method.
dags_to_latest_runs[dag.dag_id]): | ||
payload[dag.safe_dag_id] = { | ||
'dag_id': dag.dag_id, | ||
'last_run': dags_to_latest_runs[dag.dag_id].strftime("%Y-%m-%d %H:%M") |
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.
This should probably be .isoformat()
? (or at least include seconds and MS)
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.
That might make the UI look too crowded, and not sure if there is value to users to have this additional information. This PR maintains the same format as before in the non-async version: https://github.com/apache/airflow/pull/5339/files#diff-f38558559ea1b4c30ddf132b7f223cf9L111
1cc5ef2
to
2f03e26
Compare
Addressed feedback @ashb |
last_run = json[safe_dag_id].last_run; | ||
g = d3.select('div#last-run-' + safe_dag_id) | ||
g.selectAll('a') | ||
.attr("href", "{{ url_for('Airflow.graph') }}?dag_id=" + encodeUriComponent(dag_id) + "&execution_date=" + last_run) |
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.
typo: encodeURIComponent
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.
Whops that was my bad.
lgtm |
2f03e26
to
693e141
Compare
Good catch Sumit! Fixed and tested. Not your fault Ash, falls on me, but I usually do UI tests after back-and-forth comments so would have caught this at the end. |
Unreverting AIRFLOW-3160 which was reverted on suspicious of breaking CI without the clear blame of this PR, and the logs are gone now. A lot of the test code has changed significantly so it's possible that even if an issue was caused by this PR it has since been fixed. Twitter has been running for several months now with this patch with no issues.
Here is the link to the original PR: #4005
Here is the link to the revert PR: #4145
Tested on a local webserver after rebasing the unrevert, and no issues.
I am going to kick off 10 builds in Travis to see if the issue reproduces, and watch the master build for this issue for a couple of days once this is merged. Here are my builds: https://travis-ci.org/aoen/incubator-airflow/builds
@ashb