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-1561] Fix scheduler to pick up example DAGs without other DAGs #2635

Merged
merged 1 commit into from Nov 25, 2018

Conversation

mrkm4ntr
Copy link
Contributor

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    The scheduler doesn't pick up example dags unless there is at least 1 dag in dags folder. This is confusing for those who try it for the first time.

Tests

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

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"

@codecov-io
Copy link

codecov-io commented Sep 26, 2017

Codecov Report

Merging #2635 into master will increase coverage by 0.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
+ Coverage   77.81%   77.83%   +0.01%     
==========================================
  Files         201      201              
  Lines       16350    16351       +1     
==========================================
+ Hits        12723    12726       +3     
+ Misses       3627     3625       -2
Impacted Files Coverage Δ
airflow/jobs.py 77.36% <0%> (+0.27%) ⬆️
airflow/utils/dag_processing.py 57.85% <100%> (+0.32%) ⬆️
airflow/models.py 92.28% <100%> (-0.05%) ⬇️

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 77da1cc...d1f6f55. Read the comment docs.

if include_examples:
example_dag_folder = os.path.join(
os.path.dirname(os.path.dirname(__file__)),
'example_dags')
Copy link
Contributor

@edgarRd edgarRd Sep 29, 2017

Choose a reason for hiding this comment

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

Would be good to have literal 'example_dags' as a common constant.

@xnuinside
Copy link
Contributor

xnuinside commented Nov 22, 2018

@mrkm4ntr, will you update PR and squash commits?
@kaxil, @ashb, can you review this PR? It was relative to several tasks, including https://issues.apache.org/jira/projects/AIRFLOW/issues/AIRFLOW-3118

if the author will not answer, maybe make sense just reopen it as PR for https://issues.apache.org/jira/projects/AIRFLOW/issues/AIRFLOW-3118

@mrkm4ntr
Copy link
Contributor Author

mrkm4ntr commented Nov 22, 2018

@xnuinside Sure. However I would like to start after the reviewers are assigned.

@kaxil
Copy link
Member

kaxil commented Nov 22, 2018

Looks logical, can you rebase the master please. Once that is done I will test this and if everything works file, will merge 👍

@mrkm4ntr
Copy link
Contributor Author

@kaxil I rebased. Please check it.

@kaxil kaxil requested review from Fokko and ashb November 23, 2018 17:36
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

LGTM

@kaxil
Copy link
Member

kaxil commented Nov 23, 2018

@ashb @Fokko Any comments, if none - I'll merge this. I tested this PR and it works fine and fixes the issue.

A minor issue I have is "it says 16-17 dags in DAGBAG" when load_examples = True and there are no dags in DAGS folder. It says that because of the example DAGs but might be confusing for users. However, I don't have a major issue with that.

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.

LGTM, much cleaner

@Fokko Fokko merged commit 07ab945 into apache:master Nov 25, 2018
tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
@XD-DENG
Copy link
Member

XD-DENG commented Nov 30, 2018

Hi @Fokko @kaxil , seems the Travis CI testing time needed for the two backend-sqlite tests increased from ~30 minutes to ~40 minutes since this PR is merged.

May worth having a check.

@ashb
Copy link
Member

ashb commented Nov 30, 2018

We probably need to not disable loading all DAGs and instead only specific dags in many tests.

@XD-DENG
Copy link
Member

XD-DENG commented Nov 30, 2018

The weird thing is only the two backedn-sqlite tests are affected. The time needed by mysql/postgres tests remain roughly the same.

@kaxil
Copy link
Member

kaxil commented Nov 30, 2018

Good find @XD-DENG , will need to look into it and as Ash mentioned we might want to stop loading all dags

elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
ashb pushed a commit to ashb/airflow that referenced this pull request Jan 10, 2019
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Jan 23, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
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.

None yet

8 participants