Skip to content

[AIRFLOW-2430] Extend query batching to additional slow queries#3324

Closed
gsilk wants to merge 1 commit into
apache:masterfrom
gsilk:batch-inserts
Closed

[AIRFLOW-2430] Extend query batching to additional slow queries#3324
gsilk wants to merge 1 commit into
apache:masterfrom
gsilk:batch-inserts

Conversation

@gsilk

@gsilk gsilk commented May 7, 2018 •
edited
Loading

Copy link
Copy Markdown

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This fix extends the query batching introduced by an earlier commit, along with a helper method for easily doing batched queries. It is necessary to help scale the scheduler to a large number of tasks. See JIRA issue for more details.

Tests

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

There is already test coverage for reset_state_for_orphaned_tasks(), and the max_tis_per_query config is already tested by test_execute_task_instances_limit

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

@gsilk
gsilk force-pushed the batch-inserts branch 3 times, most recently from 526d766 to 8bd93b4 Compare May 7, 2018 21:57
@codecov-io

codecov-io commented May 7, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3324 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3324      +/-   ##
==========================================
+ Coverage   76.11%   76.12%   +0.01%     
==========================================
  Files         197      197              
  Lines       14741    14749       +8     
==========================================
+ Hits        11220    11228       +8     
  Misses       3521     3521
Impacted Files Coverage Δ
airflow/jobs.py 82.81% <100%> (-0.02%) ⬇️
airflow/utils/helpers.py 66.66% <100%> (+1.96%) ⬆️

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 06017f6...27713de. Read the comment docs.

@gsilk
gsilk force-pushed the batch-inserts branch 2 times, most recently from 08f08a9 to 27713de Compare May 10, 2018 02:26
@bolkedebruin

bolkedebruin commented May 10, 2018

Copy link
Copy Markdown
Contributor

Nice! Can you add some unit tests to the new functions you have added in helpers?

@gsilk

gsilk commented May 10, 2018

Copy link
Copy Markdown
Author

Thanks for the feedback! I added a few unit tests, as well as sanity checks for the chunk_size argument in the helper methods.

Fokko
Fokko requested changes May 10, 2018
View reviewed changes
Comment thread airflow/jobs.py Outdated

@Fokko Fokko May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe set a default value in this for the configuration templates?

@gsilk gsilk May 10, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My PR uses an existing config parameter that was used to implement batching, but for only -one- of the problematic queries. I've simply refactored and extended that approach.

So, the config parameter is actually already in the template, but the default is 0.

If we leave it at 0, Airflow's current behavior will be preserved. However, it might make sense to change the default to 512, which I've found to work well in practice.

WDYT?

@Fokko Fokko May 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please add it to the config with a description what the configuration parameter does. I think it would be good to set it to 512, since it will improve performance.

@gsilk gsilk May 11, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've set it to 512 as a default, and added a more detailed comment

@gsilk
gsilk force-pushed the batch-inserts branch 3 times, most recently from 66ad43c to 239a4f2 Compare May 10, 2018 21:22
Fokko
Fokko approved these changes May 13, 2018
View reviewed changes

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @gsilk LGTM

@asfgit asfgit closed this in 042c3f2 May 13, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
[AIRFLOW-2430] Extend query batching to additional slow queries
79e973c 
Closes apache#3324 from gsilk/batch-inserts
@Taragolis Taragolis mentioned this pull request Feb 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fokko Fokko Fokko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gsilk @codecov-io @bolkedebruin @Fokko