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-7063] Fix dag.clear() slowness caused by count #7723

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

yuqian90
Copy link
Contributor

@yuqian90 yuqian90 commented Mar 14, 2020

Calling tis.count() makes dag.clear() much slower than just retrieving all the tis and call len() because of some issues with how sqlalchemy generate the sql for count() when the query has many UNION statements. See the comments on the JIRA for detailed performance timing.


Issue link: AIRFLOW-7063

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Fantastic improvement @yuqian90 ! . Who would have thought sqlalchemy is that bad at counting :)! @mik-laj - you might find it interesting for your performance changes (and future backporting)

@turbaszek turbaszek requested a review from mik-laj March 14, 2020 13:30
@yuqian90
Copy link
Contributor Author

yuqian90 commented Mar 14, 2020

Thanks for the review. The test I added does not play well with sqlite. I'll fix soon

@yuqian90 yuqian90 closed this Mar 14, 2020
@yuqian90 yuqian90 reopened this Mar 14, 2020
@turbaszek
Copy link
Member

Thanks for the review. The test I added does not play well with sqlite. I'll fix soon

The error is really interesting 🤔

@yuqian90
Copy link
Contributor Author

Thanks for the review. The test I added does not play well with sqlite. I'll fix soon

The error is really interesting 🤔

The test failed in sqlite because of a hard limit on the level of nesting in the query. The test is specifically made to cause many levels of nesting in the generated sql to reproduce the sqlalchemy slowness. Stackoverflow suggests there are compiler options to increase this hard limit in sqlite, but I doubt anyone uses sqlite in production with a dag this complicated. I think in this case skipping the test in sqlite backend is the right thing to do so I did that with pytest.mark.backend.

@potiuk
Copy link
Member

potiuk commented Mar 14, 2020

Indeed. Interesting one :)

@codecov-io
Copy link

codecov-io commented Mar 14, 2020

Codecov Report

Merging #7723 into master will decrease coverage by 0.76%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7723      +/-   ##
==========================================
- Coverage   86.92%   86.15%   -0.77%     
==========================================
  Files         915      915              
  Lines       44152    44152              
==========================================
- Hits        38377    38041     -336     
- Misses       5775     6111     +336     
Impacted Files Coverage Δ
airflow/models/dag.py 91.21% <100.00%> (ø)
...flow/providers/apache/cassandra/hooks/cassandra.py 21.51% <0.00%> (-72.16%) ⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py 35.84% <0.00%> (-64.16%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0.00%> (-55.56%) ⬇️
airflow/providers/postgres/operators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
airflow/providers/redis/operators/redis_publish.py 50.00% <0.00%> (-50.00%) ⬇️
airflow/kubernetes/volume.py 52.94% <0.00%> (-47.06%) ⬇️
airflow/providers/mongo/sensors/mongo.py 53.33% <0.00%> (-46.67%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0.00%> (-45.08%) ⬇️
airflow/providers/mysql/operators/mysql.py 55.00% <0.00%> (-45.00%) ⬇️
... and 12 more

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 e31e9dd...51879a5. Read the comment docs.

@potiuk potiuk merged commit 6fc5148 into apache:master Mar 14, 2020
@potiuk
Copy link
Member

potiuk commented Mar 14, 2020

Thanks @yuqian90 !

potiuk pushed a commit that referenced this pull request Mar 14, 2020
kaxil pushed a commit that referenced this pull request Mar 19, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 19, 2020
@yuqian90 yuqian90 deleted the slow_clear branch September 16, 2020 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants