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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-2086] Add a configuration variable to control numbers of dag run for display #3279

Closed
wants to merge 1 commit into from
Closed

[AIRFLOW-2086] Add a configuration variable to control numbers of dag run for display #3279

wants to merge 1 commit into from

Conversation

feng-tao
Copy link
Member

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"

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Similar to what airflow-2393, airflow-2086 describes, airflow tree view is very slow to run which often triggers guicorn timeout if the dag has many tasks and dependencies. We limit the default tree view to most recent 5 dagruns which help to alleviate the issue.

Tests

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

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
    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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@codecov-io
Copy link

codecov-io commented Apr 29, 2018 •
edited
Loading

Codecov Report

Merging #3279 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3279      +/-   ##
==========================================
+ Coverage   76.07%   76.11%   +0.04%     
==========================================
  Files         197      197              
  Lines       14733    14741       +8     
==========================================
+ Hits        11208    11220      +12     
+ Misses       3525     3521       -4
Impacted Files Coverage Δ
airflow/www_rbac/views.py 72.47% <100%> (+0.09%) ⬆️
airflow/www/views.py 71.46% <100%> (+0.07%) ⬆️
airflow/models.py 86.68% <0%> (+0.04%) ⬆️
airflow/jobs.py 82.83% <0%> (+0.26%) ⬆️

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 2728138...03e241c. Read the comment docs.

Fokko
Fokko requested changes Apr 30, 2018
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I would suggest to make this configurable. Right now I only see a lot of magic variables ;)

@milanvdm
Copy link
Contributor

milanvdm commented May 4, 2018

I would like to have this included in the upcoming release as we have clients with a poor internet connection and showing 25 dags as default is slowing us down.

Making it configurable is IMHO another kind of PR and is not relevant to this one.

@feng-tao
Copy link
Member Author

feng-tao commented May 6, 2018

Hey @Fokko , I update the pr to add a new config for default_dag_run :)

@feng-tao
Copy link
Member Author

feng-tao commented May 7, 2018

ping @Fokko and @mistercrunch

artwr
artwr reviewed May 7, 2018
View reviewed changes
Copy link
Contributor

@artwr artwr left a comment

Choose a reason for hiding this comment

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

@feng-tao I am not completely sure about making the default 5. I agree with a configurable default so that Airflow admins can decide for themselves how many makes sense depending on the typical shape of their DAGs. But a beginner running a small backfill would most likely want a little bit more history than just 5 runs.

airflow/config_templates/default_airflow.cfg Outdated
@@ -280,6 +280,9 @@ rbac = False
# Define the color of navigation bar
navbar_color = #007A87

# Default dagrun to show in UI
default_dag_run = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

default_dag_run_display_number maybe?

@Fokko
Copy link
Contributor

Fokko commented May 8, 2018

I agree with @artwr, maybe set the default to 25?

@artwr
Copy link
Contributor

artwr commented May 8, 2018

I think the default of 25 is right. If it is really too much we could drop it down to 15 (just over 2 weeks), but then 15 needs to be in the drop-down.

@feng-tao
Copy link
Member Author

feng-tao commented May 8, 2018

sure, let me update the pr back to 25 later today.

@feng-tao
Copy link
Member Author

feng-tao commented May 9, 2018

hey @artwr and @Fokko , I update the pr by changing the default to 25. Would you mind taking another look? Thanks.

@feng-tao feng-tao changed the title [AIRFLOW-2086] Limit default dagrun to 5 in view [AIRFLOW-2086] Add a configuration variable to control numbers of dag run for display May 9, 2018
@artwr
Copy link
Contributor

artwr commented May 9, 2018

LGTM +1

@asfgit asfgit closed this in 2a55ffe May 9, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
[AIRFLOW-2086][AIRFLOW-2393] Customize default dagrun number in tree …
a70c6cc 
…view

Closes apache#3279 from feng-tao/reduce-tree-view

This introduces a new configuration variable to set the default
number of dag runs displayed in the tree view. For large DAGs, this
could cause timeouts in the webserver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artwr artwr artwr left review comments

@Fokko Fokko Fokko requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@feng-tao @codecov-io @milanvdm @Fokko @artwr