Change config max tis per query by lyso · Pull Request #29602 · apache/airflow

lyso · 2023-02-18T04:07:34Z

This PR changes the default value for configure scheduler.max_tis_per_query to 16 and update related document.

Before this change scheduler.max_tis_per_query=512 by default.

scheduler.max_tis_per_query should not be greater than core.parallelism according to related code [1] and [2]. If both configurations use default value, the default 512 of max_tis_per_query is never used.

After this PR, default will set scheduler.max_tis_per_query=16 and core.parallelism=32. This will make the executor can be populated in two iteration of scheduler main loop.

The actual change applied in Airflow runtime by this PR is to reduce the max_tis from 32 to 16. The query batch size reduced. This can help scheduler finish the iteration faster. Thus make scheduler more responsive and healthy.

This PR also updated related document about scheduler.max_tis_per_query to help users understand how to set this configuration.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

boring-cyborg · 2023-02-18T04:07:37Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

Taragolis · 2023-02-18T05:31:04Z

Thank for your contributions.

Could you describe a bit more of configurations of Airflow, your DB backend, how far it away out of Airflow services (latency), and your workload. I asked it because for me it is looks like by this PR we might replace one magic number by another.

I've also found that max_tis_per_query use in BackfillJob:

airflow/airflow/jobs/backfill_job.py

Lines 957 to 959 in aa4858d

    
           reset_tis = helpers.reduce_in_chunks(query, tis_to_reset, [], self.max_tis_per_query)

Do you know how it affects this part of code?

dimberman · 2023-02-23T19:57:13Z

Hi @lyso not a big deal but in the future please don't put your name in the PR title.

lyso · 2023-03-04T10:40:16Z

Thank for your contributions.

Could you describe a bit more of configurations of Airflow, your DB backend, how far it away out of Airflow services (latency), and your workload. I asked it because for me it is looks like by this PR we might replace one magic number by another.

I've also found that max_tis_per_query use in BackfillJob:

airflow/airflow/jobs/backfill_job.py

Lines 957 to 959 in aa4858d

reset_tis = helpers.reduce_in_chunks(query, tis_to_reset, [], self.max_tis_per_query)

Do you know how it affects this part of code?

I use MWAA medium class, so DB is RDS posgreSQL, 2xSchedulers run on Fargate Container with 2vCPU and 4GB RAM. Executor is CeleryExecutor via SQS.

At worst case, there are 200-300 tasks scheduled at the beginning of hours. Because of the 512 max_tis_per_query, all of these tasks into single batch of the query when a scheduler enqueues tasks. It takes about 1-2 minutes to enqueue these tasks and send out to Celery broker. Based on my observation, the bottleneck is not the DB query, but is the in-memory process of _critical_section_enqueue_task_instances and executor.heartbeat. The scheduler need 1-2 minutes to change TI state from SCHEDULED to QUEUED and then send out to broker.

During the 1-2 minutes period, the scheduler cannot make heartbeat and could be detected as inactive, then possibly causing unexpected behaviors like orphaned task adoption and task external kills.

Furthermore, in case of 2 or more schedulers (HA), setting max_tis_per_query to 512 can lead to one scheduler overloaded, but other schedulers have no tasks in their Executor (if core.parallelism is also very big).

When tuning the parameter, I lowered the max_tis_per_query to 50, then the entire cluster become much healthier. Schedulers can quickly put a batch of tasks to Celery broker. Scheduler Heartbeat rate is improved. And, more importantly, load are shared between 2 schedulers.

When submitting the value in this PR, I put the magic number as 16 as default for max_tis_per_query. I did not choose the value I use in my cluster, because I believe this value should be adjusted based on the hardware configuration. 16 is a value given with respect to core.parallelism (default 32). When setting core.parallelism=32, we assume the hardware of scheduler and database is not very powerful. Optimally max_tis_per_query should be a divisor of core.parallelism. 8 would be too small. So I chose 16.

About `max_tis_per_query` in Backfill job

It is used in reset orphaned task method. The helpers.reduce_in_chunks will split the entire list into smaller chunks (of size max_tis_per_query, and query DB per chunk.

I believe 16 is also a good setting here. Since the core.parallelism's default is 32, we assume this is optimal for the assumed hardware. In this case, having the query batch size as 512 would be too big for the assumed hardware.

Taragolis · 2023-03-04T13:03:54Z

@lyso That is pretty good explanation why potentially lower value better than current 512, especially with HA schedulers

…wer value 16 and udpate description.

github-actions · 2023-04-21T00:11:12Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

o-nikolas · 2023-04-21T20:18:29Z

Commenting on this one to ensure it doesn't get aurtoclosed. It has a very nice performance enhancement!

@lyso What is the current status of this PR? Looks like there are some static checks that need fixing and some tests are failing as well.

@dimberman Can you have another look, your requested change has been addressed.

potiuk · 2023-04-21T20:32:00Z

Commenting on this one to ensure it doesn't get aurtoclosed. It has a very nice performance enhancement!

You can add pinned label to prevent a PR to be closed (just did).

…ig function

lyso · 2023-04-29T22:17:45Z

Commenting on this one to ensure it doesn't get aurtoclosed. It has a very nice performance enhancement!

@lyso What is the current status of this PR? Looks like there are some static checks that need fixing and some tests are failing as well.

@dimberman Can you have another look, your requested change has been addressed.

The errors has been addressed.

@dimberman any comments on your feature requests?

potiuk · 2023-04-29T22:21:40Z

The errors has been addressed.

Let's see. Approved the run now.

potiuk · 2023-04-29T23:01:06Z

There are still static check problems. I recommend installing pre-commits and running them.

potiuk · 2023-05-02T12:09:22Z

@dimberman - it's still blocked by your "request changes".

uranusjr · 2023-05-03T21:17:03Z

+            # in case any of the above config not found, return directly
+            # this could happens in some unit tests


Which unit tests? We can add these to them with conf_vars.

Here are the tests failed with this config key not found:

FAILED tests/core/test_configuration.py::TestConf::test_auth_backends_adds_session - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_values_from_conf - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_env_vars_upgraded_and_removed[old0-new0] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_env_vars_upgraded_and_removed[old1-new1] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_values_from_environ[conf_dict0] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_values_from_environ[conf_dict1] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_sections[old_config] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_sections[old_config_old_env] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_sections[old_env] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config FAILED tests/core/test_configuration.py::TestDeprecatedConf::test_deprecated_sections[new_config_old_env] - airflow.exceptions.AirflowConfigException: section/key [scheduler/max_tis_per_query] not found in config

These test cases are not using; conf_vars to initializing the conf. Instead, they initialize conf from a string directly, like test_conf = AirflowConfigParser(default_config=""). I believe it has been done for purpose.

Can we not add the needed config to the string?

Can we not add the needed config to the string?

Adding to their configuration string makes them dependent on this module.

These are unit tests. I don't think it's appropriate tho introduce this dependence to them.

But the implementation those tests cover does depend on the configuration. If you do not want they to depend on it, your implementation to the config needs to be changed.

@lyso do you want to make those changes ? Or @uranusjr would you be OK leaving the code as is ?

vandonr-amz · 2023-07-05T18:30:19Z

@dimberman you still have changes requested on this PR, but I think your concerns have been addressed, would you mind giving it an other look ?

eladkal · 2023-07-20T19:03:41Z

Closed in favor of #32572

boring-cyborg Bot added area:Scheduler including HA (high availability) scheduler kind:documentation labels Feb 18, 2023

Taragolis added the pending-response label Feb 19, 2023

dimberman changed the title ~~Lyso change config max tis per query~~ Change config max tis per query Feb 23, 2023

dimberman requested changes Feb 23, 2023

View reviewed changes

Comment thread airflow/config_templates/default_airflow.cfg Outdated

lyso requested a review from potiuk as a code owner March 5, 2023 10:14

lyso requested review from ashb, jedcunningham and kaxil as code owners March 6, 2023 06:39

lyso requested review from dimberman and removed request for ashb, jedcunningham, kaxil and potiuk March 6, 2023 06:41

lyso force-pushed the lyso-change-config-max_tis_per_query branch from 6b33933 to 1c67920 Compare March 6, 2023 06:53

lyso requested review from XD-DENG, bbovenzi, dstandish, eladkal, ephraimbuddy, josh-fell, mik-laj, o-nikolas, ryanahamilton and uranusjr as code owners March 6, 2023 07:06

simonlju-amazon and others added 3 commits March 6, 2023 18:07

changing default value for config scheduler.max_tis_per_query to a lo…

0419d0d

…wer value 16 and udpate description.

update document for changing config max_tis_per_query default to 16

d0b5f9f

Add validation and warn message for max tis per query

1c4d597

lyso force-pushed the lyso-change-config-max_tis_per_query branch from 6632c84 to 1c4d597 Compare March 6, 2023 07:07

github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 21, 2023

potiuk added the pinned Protect from Stalebot auto closing label Apr 21, 2023

eladkal removed stale Stale PRs per the .github/workflows/stale.yml policy file pending-response labels Apr 21, 2023

root and others added 3 commits April 29, 2023 22:14

update config.yml to match config.cfg, and run black formatter

f4a897a

Merge branch 'apache:main' into lyso-change-config-max_tis_per_query

0def86a

fix the exception raised in some unit test from the new validate conf…

04e34c1

…ig function

lyso added 4 commits April 30, 2023 17:19

remove tailing whitespaces in scheduler.rst

b8efa24

update config.yml descript to align with default_airflow.cfg

aacb800

Merge branch 'apache:main' into lyso-change-config-max_tis_per_query

b48373e

Merge branch 'main' into lyso-change-config-max_tis_per_query

b9b2bfa

lyso requested a review from Taragolis May 2, 2023 12:44

Merge branch 'main' into lyso-change-config-max_tis_per_query

eb69403

uranusjr reviewed May 3, 2023

View reviewed changes

vandonr-amz mentioned this pull request Jul 12, 2023

Reduce config's default for max TIs per query & enforce <= parallelism #32572

Merged

eladkal closed this Jul 20, 2023

		# in case any of the above config not found, return directly
		# this could happens in some unit tests

Uh oh!

Conversation

lyso commented Feb 18, 2023

Uh oh!

boring-cyborg Bot commented Feb 18, 2023

Uh oh!

Taragolis commented Feb 18, 2023

Uh oh!

dimberman commented Feb 23, 2023

Uh oh!

Uh oh!

lyso commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About max_tis_per_query in Backfill job

Uh oh!

Taragolis commented Mar 4, 2023

Uh oh!

github-actions Bot commented Apr 21, 2023

Uh oh!

o-nikolas commented Apr 21, 2023

Uh oh!

potiuk commented Apr 21, 2023

Uh oh!

lyso commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Apr 29, 2023

Uh oh!

potiuk commented Apr 29, 2023

Uh oh!

potiuk commented May 2, 2023

Uh oh!

uranusjr May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyso May 4, 2023

Choose a reason for hiding this comment

Uh oh!

uranusjr May 8, 2023

Choose a reason for hiding this comment

Uh oh!

lyso May 27, 2023

Choose a reason for hiding this comment

Uh oh!

uranusjr May 28, 2023

Choose a reason for hiding this comment

Uh oh!

vandonr-amz Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

vandonr-amz commented Jul 5, 2023

Uh oh!

eladkal commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lyso commented Mar 4, 2023 •

edited

Loading

About `max_tis_per_query` in Backfill job

lyso commented Apr 29, 2023 •

edited

Loading

uranusjr May 3, 2023 •

edited

Loading