[AIRFLOW-5543] Fix tooltip disappears in tree and graph view #6174

pingzh · 2019-09-23T19:26:57Z

when page scrolls

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-5543
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Here are some details about my PR, including screenshots of any UI changes:

Before: if you scroll up your page in tree view or graph view, the tooltip will also move up and in the end disappear as it is out of the page.

Using the d3-tip as other pages also use d3-tip. After the fix with page scrolling up:

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
UI updates

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@KevinYang21

codecov-io · 2019-09-23T20:26:41Z

Codecov Report

Merging #6174 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6174      +/-   ##
==========================================
+ Coverage   79.99%   80.02%   +0.02%     
==========================================
  Files         610      610              
  Lines       35176    35176              
==========================================
+ Hits        28140    28149       +9     
+ Misses       7036     7027       -9

Impacted Files	Coverage Δ
airflow/utils/dag_processing.py	`56.72% <0%> (+0.17%)`	⬆️
airflow/models/taskinstance.py	`93.75% <0%> (+0.5%)`	⬆️
airflow/jobs/backfill_job.py	`91.43% <0%> (+1.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e856e...dd8ca7c. Read the comment docs.

KevinYang21

LGTM but will leave it open for a bit longer to allow others with better frontend knowledge to review.

feng-tao · 2019-09-24T03:27:13Z

BTW, just curious @KevinYang21 has Airbnb migrated to latest Airflow version? If so, could you share more on your experiences on your process especially on how to make it less downtime or zero downtime if possible? We(Lyft) have a new cluster running for a couple new use cases, but still have been figured out a plan to migrate the legacy one to new cluster without downtime.

ashb · 2019-09-24T10:14:39Z

airflow/www/templates/airflow/graph.html

@@ -128,6 +129,17 @@
        'queued': false,
        'no_status': false
    };
+    const taskTip = d3.tip()
+      .attr('class', 'tooltip')
+      .style('background', 'black')


Rather than defining the stlyes in the JS can we put them in a .css file (as we already have given this a class)

hmm, really, tooltip is a class from bootstrap. will take a look

airflow/www/templates/airflow/graph.html

airflow/www/templates/airflow/tree.html

ashb · 2019-09-24T10:19:56Z

airflow/www/static/js/graph.js

-        return tt;
+        taskTip.show(tt, this); // taskTip is defined in graph.html
+      })
+      .on('mouseout', function(d,i) {


Can be simpler:

Suggested change

.on('mouseout', function(d,i) {

.on('mouseout', taskTip.hide))

ashb · 2019-09-24T10:22:50Z

airflow/www/templates/airflow/tree.html

+  .style("border-width", "1px")
+  .style("border-radius", "5px")
+  .style("padding", "10px")
+  .direction(function(toolTipHtml) {


I don't like this - it feels error-prone. Can we instead to tip.direction(condition ? 'e': 'n') from inside the on mouseover event handler.

Or, alternatively: we could do taskTip.show(d, this) (d here is the data/task struct from on-mouseover, and then have html/direction look at the object, not the string:

.direction(function(dl) { return d.depends_on_paste ? 'e' : 'n' }) .html(function(d) { if (d.operator != undefined) { if (d.operator != undefined) { tt += "operator: " + escapeHtml(d.operator) + " "; } tt += "depends_on_past: " + escapeHtml(d.depends_on_past) + " "; tt += "upstream: " + escapeHtml(d.num_dep) + " "; tt += "retries: " + escapeHtml(d.retries) + " "; tt += "owner: " + escapeHtml(d.owner) + " "; tt += "start_date: " + escapeHtml(d.start_date) + " "; tt += "end_date: " + escapeHtml(d.end_date) + " "; } return tt });

(untested)

that was my original design. but i did not like in the end as d is not always the object.

love tip.direction(condition ? 'e': 'n')

pingzh · 2019-09-24T17:58:44Z

@ashb addressed your comment in the second commit. thanks

pingzh · 2019-09-24T18:02:47Z

@feng-tao we upgraded one of our clusters to 1.10.4 from 1.10.0. We planned a downtime to do it as we noticed the payload of task sent to the queue was different between those airflow versions. so we cannot have two different airflow versions running. Not sure about your current airflow version in production. If you use ElasticSearch as your log backend, be sure to cherry pick this commit: https://github.com/apache/airflow/pull/6159/files

feng-tao · 2019-09-24T19:43:40Z

@pingzh interesting, so Airbnb's Data repo (or the one that is giant shared across different team) has been moved to 1.10 from 1.8.2? How's the experience ? Or how long the downtime and anything to watch out for during the upgrade?

And curious how many people in Airbnb working on Airflow core currently? Maybe our two team should have a sync together to share more the experiences :)

cc @KevinYang21

when page scrolls

pingzh · 2019-09-27T03:35:20Z

Hi @ashb , could you please take a look? thanks

KevinYang21 · 2019-09-30T01:11:20Z

@feng-tao For sure let's connect! With @saguziel recenlty returned to the team 🎉, we have 6 people working on core Airflow now.

We did migrate from from 1.8.0 to 1.10.0 eariler this year and recently migrated to 1.10.4 for one of our cluster as @pingzh mentioned. We tried to migrate w/o downtime for 1.8 -> 1.10, it was possible in theory but we wanted a smoothier migration so we spun up two cluster and rolling migrated the DAGs. We did learn quite a few lessons during the migration and would love to share them during our sync.

KevinYang21 · 2019-09-30T18:06:09Z

Hi @ashb, does @pingzh's recent change look good to you? Wanna make sure you're good with it before I merge it 🙏

feng-tao · 2019-09-30T18:13:10Z

@KevinYang21 that's great, sent you an email :)

ashb

LGTM - sorry slipped my mind.

when page scrolls

fixup! [AIRFLOW-5477] Rewrite Google PubSub Hook to Google Cloud Python fixup! fixup! [AIRFLOW-5477] Rewrite Google PubSub Hook to Google Cloud Python Apply suggestions from code review Co-Authored-By: Kamil Breguła <[email protected]> fixup! Apply suggestions from code review fixup! fixup! Apply suggestions from code review fixup! fixup! fixup! Apply suggestions from code review fixup! fixup! fixup! fixup! Apply suggestions from code review [AIRFLOW-XXX] Fix AzureContainerInstancesOperator example (apache#6225) [AIRFLOW-5543] Fix tooltip disappears in tree and graph view (apache#6174) when page scrolls [AIRFLOW-5362] Reorder imports Updated readme with "Who's using" information We are using Airflow at King Abdullah Petroleum Studies and Research Center(KAPSARC)(www.kapsarc.org)

#6174) when page scrolls (cherry-picked from commit 02b478e)

KevinYang21 approved these changes Sep 23, 2019

View reviewed changes

pingzh requested a review from KevinYang21 September 24, 2019 00:02

KevinYang21 approved these changes Sep 24, 2019

View reviewed changes

feng-tao approved these changes Sep 24, 2019

View reviewed changes

ashb requested changes Sep 24, 2019

View reviewed changes

feluelle added the core:UI label Sep 24, 2019

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view

dd8ca7c

when page scrolls

ashb approved these changes Oct 1, 2019

View reviewed changes

KevinYang21 merged commit 02b478e into apache:master Oct 1, 2019

pingzh deleted the use-d3-tip branch October 1, 2019 18:11

kaxil pushed a commit that referenced this pull request Oct 2, 2019

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view (#6174)

4dd24a2

when page scrolls

ashb pushed a commit that referenced this pull request Oct 7, 2019

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view (RBAC UI) (

89a88a6

#6174) when page scrolls (cherry-picked from commit 02b478e)

mik-laj added area:webserver Webserver related Issues and removed area:UI labels Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view #6174

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view #6174

pingzh commented Sep 23, 2019

codecov-io commented Sep 23, 2019 •

edited

Loading

KevinYang21 left a comment

feng-tao commented Sep 24, 2019

ashb Sep 24, 2019

pingzh Sep 24, 2019

ashb Sep 24, 2019

pingzh Sep 24, 2019

ashb Sep 24, 2019

ashb Sep 24, 2019

pingzh Sep 24, 2019

pingzh Sep 24, 2019

pingzh commented Sep 24, 2019

pingzh commented Sep 24, 2019

feng-tao commented Sep 24, 2019 •

edited

Loading

pingzh commented Sep 27, 2019

KevinYang21 commented Sep 30, 2019

KevinYang21 commented Sep 30, 2019

feng-tao commented Sep 30, 2019

ashb left a comment

	.on('mouseout', function(d,i) {
	.on('mouseout', taskTip.hide))

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view #6174

[AIRFLOW-5543] Fix tooltip disappears in tree and graph view #6174

Conversation

pingzh commented Sep 23, 2019

Jira

Description

Tests

Commits

Documentation

codecov-io commented Sep 23, 2019 • edited Loading

Codecov Report

KevinYang21 left a comment

Choose a reason for hiding this comment

feng-tao commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pingzh commented Sep 24, 2019

pingzh commented Sep 24, 2019

feng-tao commented Sep 24, 2019 • edited Loading

pingzh commented Sep 27, 2019

KevinYang21 commented Sep 30, 2019

KevinYang21 commented Sep 30, 2019

feng-tao commented Sep 30, 2019

ashb left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 23, 2019 •

edited

Loading

feng-tao commented Sep 24, 2019 •

edited

Loading