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

[AIRFLOW-6357] Highlight nodes in Graph UI if task id contains dots #6904

Merged
merged 1 commit into from
Dec 28, 2019
Merged

[AIRFLOW-6357] Highlight nodes in Graph UI if task id contains dots #6904

merged 1 commit into from
Dec 28, 2019

Conversation

Khrol
Copy link
Contributor

@Khrol Khrol commented Dec 26, 2019

If task id contains dots:
graph?dag_id=daily_dag:1246 Uncaught TypeError: Cannot read property 'parentNode' of null
at graph?dag_id=daily_dag:1246
at Array.forEach ()
at highlight_nodes (graph?dag_id=daily_dag:1245)
at SVGGElement. (graph?dag_id=daily_dag:1253)
at SVGGElement.__onmouseover (d3.min.js:1)

https://stackoverflow.com/a/25942226/1849364 - some more context about the fix.

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-6357
    • 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

The following DAG helps to reproduce the problem:

with DAG(dag_id='hightlight_test', start_date=airflow.utils.dates.days_ago(2)) as dag:
    t1 = DummyOperator(task_id='t1')
    t2 = DummyOperator(task_id='t2.t2')
    t3 = DummyOperator(task_id='t3.t3')
    t1 >> t2
    t2 >> t3
  • Here are some details about my PR, including screenshots of any UI changes:
    After the fix:
    Screen Shot 2019-12-26 at 13 01 43

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    I don't know if we are able to cover it by UI tests.

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

If task id contains dots:
graph?dag_id=daily_dag:1246 Uncaught TypeError: Cannot read property 'parentNode' of null
at graph?dag_id=daily_dag:1246
at Array.forEach (<anonymous>)
at highlight_nodes (graph?dag_id=daily_dag:1245)
at SVGGElement.<anonymous> (graph?dag_id=daily_dag:1253)
at SVGGElement.__onmouseover (d3.min.js:1)

https://stackoverflow.com/a/25942226/1849364 - some more context about the fix.
@codecov-io
Copy link

Codecov Report

Merging #6904 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6904   +/-   ##
=======================================
  Coverage   84.72%   84.72%           
=======================================
  Files         679      679           
  Lines       38505    38505           
=======================================
  Hits        32623    32623           
  Misses       5882     5882

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 947fb64...aeb0815. Read the comment docs.

@kaxil
Copy link
Member

kaxil commented Dec 27, 2019

Why do we need this @Khrol ? What is the use-case or purpose it serves?

Sorry not against it but want to understand the reasoning behind it

@Khrol
Copy link
Contributor Author

Khrol commented Dec 28, 2019

@kaxil , it's a bug fix. Normally, task highlighting works fine in Graph UI when you move the mouse over tasks (with direct upstream and downstream). If upstream or downstream task id contains dots, an exception is thrown and it's not highlighted.

We use dots in task ids to keep tasks corresponding to Hive table names. E.g. some_database.table_name.

@kaxil
Copy link
Member

kaxil commented Dec 28, 2019

Got it

@kaxil kaxil merged commit 365a424 into apache:master Dec 28, 2019
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
@Khrol Khrol deleted the AIRFLOW-6357_graph_ui_highlight branch September 1, 2020 06:48
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