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-2861] Added index on log table #3709

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
5 participants
@vardancse
Copy link
Contributor

vardancse commented Aug 6, 2018

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:
    Delete dag functionality is added in v1-10-stable, whose implementation during the metadata cleanup part, look for classes which has attribute named as dag_id and then formulate the query on matching model and then delete from metadata, we've few numbers where we've observed slowness especially in log table because it doesn't have any single or multiple-column index. Creating an index would boost the performance though insertion will be a bit slower. Since deletion will be a sync call, would be good idea to create index.

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 (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.
    • 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
@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Aug 6, 2018

I think this also requires a database migration.

@vardancse

This comment has been minimized.

Copy link
Contributor Author

vardancse commented Aug 7, 2018

@Fokko I think I might be missing something, could you please correct me why we need to do database migration? If you're talking about in existing configuration, then an index statement needs to be applied directly to metadata and if configuration is new, it will consider one mentioned in models.py

Additionally, could you please help in knowing why checks have been failed? It seems to be stopped, Can I recheck it?

@feng-tao

This comment has been minimized.

Copy link
Contributor

feng-tao commented Aug 7, 2018

@vardancse , you need to add an alemic migration script for index change(e.g https://github.com/apache/incubator-airflow/blob/master/airflow/migrations/versions/211e584da130_add_ti_state_index.py)
And you need to rebase your pr with master which will fix the CI issue.

@vardancse vardancse force-pushed the vardancse:master branch 2 times, most recently from 5706fc9 to 7d0fee4 Aug 7, 2018

@vardancse

This comment has been minimized.

Copy link
Contributor Author

vardancse commented Aug 7, 2018

@feng-tao @Fokko Thanks for reviewing and explaining process, I've added migration script. Let me know, if anything else is required.

@bolkedebruin

This comment has been minimized.

Copy link
Contributor

bolkedebruin commented Aug 7, 2018

Please correct your commit message per guidelines (shorten, remove [improvement], use imperative)

@vardancse vardancse force-pushed the vardancse:master branch from 7d0fee4 to 17790c5 Aug 7, 2018

@vardancse vardancse changed the title [AIRFLOW-2861][Improvement] Added index on log table, will make delete dag optimized [AIRFLOW-2861][Improvement] Added index on log table Aug 7, 2018

@vardancse vardancse force-pushed the vardancse:master branch from 17790c5 to 95e41ed Aug 7, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 7, 2018

Codecov Report

Merging #3709 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3709      +/-   ##
==========================================
- Coverage   77.56%   77.56%   -0.01%     
==========================================
  Files         204      204              
  Lines       15766    15767       +1     
==========================================
  Hits        12229    12229              
- Misses       3537     3538       +1
Impacted Files Coverage Δ
airflow/models.py 88.54% <100%> (-0.04%) ⬇️

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 27b436e...95e41ed. Read the comment docs.

@vardancse

This comment has been minimized.

Copy link
Contributor Author

vardancse commented Aug 7, 2018

@bolkedebruin Done, thanks!

@vardancse vardancse changed the title [AIRFLOW-2861][Improvement] Added index on log table [AIRFLOW-2861] Added index on log table Aug 7, 2018

@vardancse

This comment has been minimized.

Copy link
Contributor Author

vardancse commented Aug 8, 2018

@bolkedebruin @feng-tao @Fokko Do you guys see any other concern?

@bolkedebruin bolkedebruin merged commit 6f7fe74 into apache:master Aug 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lxneng added a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

dalupus added a commit to modmed/incubator-airflow that referenced this pull request Sep 19, 2018

ashb added a commit to ashb/airflow that referenced this pull request Dec 15, 2018

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

kaxil added a commit that referenced this pull request Jan 9, 2019

ashb added 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment