You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ashb (via GitHub)" <gi...@apache.org> on 2025/03/12 15:04:30 UTC
[PR] Make LocalExecutor work under heavy load [airflow]
ashb opened a new pull request, #47678:
URL: https://github.com/apache/airflow/pull/47678
This change seems innocuous, and possibly even wrong, but it is the correct
behaviour since #47320 landed. We _do not_ want to call dispose_orm, as that
ends up reconnecting, and sometimes this results in the wrong connection
being shared between the parent and the child. I don't love the "sometimes"
nature of this bug, but the fix seems sound.
Prior to this running one or two runs concurrently would result in the
scheduler handing (stuck in SQLA code trying to roll back) or an error from
psycopg about "error with status PGRES_TUPLES_OK and no message from the libpq".
With this change we were able to repeatedly run 10 runs concurrently.
The reason we don't want this is that we registered an at_fork handler already
that closes/discards the socket object (without closing the DB level session)
so calling dispose can, perversely, resurrect that object and try reusing it!
Co-authored-by: Jed Cunningham <66...@users.noreply.github.com>
Co-authored-by: Kaxil Naik <ka...@apache.org>
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make LocalExecutor work under heavy load [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47678:
URL: https://github.com/apache/airflow/pull/47678#discussion_r1991737924
##########
airflow/jobs/scheduler_job_runner.py:
##########
@@ -1061,14 +1061,16 @@ def _run_scheduler_loop(self) -> None:
with create_session() as session:
# This will schedule for as many executors as possible.
num_queued_tis = self._do_scheduling(session)
+ # Don't keep any objects alive -- we've possibly just looked at 500+ ORM objects!
+ session.expunge_all()
- # Heartbeat all executors, even if they're not receiving new tasks this loop. It will be
- # either a no-op, or they will check-in on currently running tasks and send out new
- # events to be processed below.
- for executor in self.job.executors:
- executor.heartbeat()
+ # Heartbeat all executors, even if they're not receiving new tasks this loop. It will be
+ # either a no-op, or they will check-in on currently running tasks and send out new
+ # events to be processed below.
+ for executor in self.job.executors:
+ executor.heartbeat()
Review Comment:
This is so that executor.heartbeat is not called during an active transaction -- given we don't pass a session on, if a specific executor wants to do DB actions it will need to create it's own session anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make LocalExecutor work under heavy load [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47678:
URL: https://github.com/apache/airflow/pull/47678#discussion_r1991739482
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -329,7 +327,7 @@ def exit(n: int) -> NoReturn:
import traceback
try:
- last_chance_stderr.write("--- Last chance exception handler ---\n")
+ last_chance_stderr.write("--- Supervised process Last chance exception handler ---\n")
Review Comment:
This was just to make the error message unambiguously from us/Airflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make LocalExecutor work under heavy load [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #47678:
URL: https://github.com/apache/airflow/pull/47678
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make LocalExecutor work under heavy load [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #47678:
URL: https://github.com/apache/airflow/pull/47678#issuecomment-2718895599
Merging. Failure is unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make LocalExecutor work under heavy load [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #47678:
URL: https://github.com/apache/airflow/pull/47678#issuecomment-2718258133
TaskSDK tests are failing in task sdk. Fix is here https://github.com/apache/airflow/pull/47679
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org