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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix tasks in an infinite slots pool were never scheduled #15247
Conversation
If typing is applied well to the related code path, declaring |
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. |
airflow/models/pool.py
Outdated
@@ -106,6 +107,8 @@ def slots_stats( | |||
|
|||
pool_rows: Iterable[Tuple[str, int]] = query.all() | |||
for (pool_name, total_slots) in pool_rows: | |||
if total_slots == -1: | |||
total_slots = maxsize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one downside to this is that it would then be logged as 9223372036854775807 slots free
etc.
It breaks/is lying about the type, but how about:
total_slots = maxsize | |
total_slots = float('inf') # type: ignore |
Because this has all the properties we want:
- Anything subtracted from it is still infinity
- It is greater than any actual number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, and existing log end up pretty solid too:
[2021-06-04 13:37:28,963] {scheduler_job.py:993} INFO - Figuring out tasks to run in Pool(name=infinite_pool) with inf open slots and 1 task instances ready to be queued
[2021-06-04 13:37:28,963] {scheduler_job.py:1021} INFO - DAG SchedulerJobTest.test_infinite_pool has 0/16 running and queued tasks
[2021-06-04 13:37:28,963] {scheduler_job.py:1086} INFO - Setting the following tasks to queued state:
<TaskInstance: SchedulerJobTest.test_infinite_pool.dummy 2016-01-01 00:00:00+00:00 [scheduled]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
bdc7719
to
9d01651
Compare
Eh, what is the bot doing? |
Strange |
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
@bperson We've got a test failure in
|
@ashb sorry about that, it should be better now ( though Ci seems to be stuck in an endless 404-ing loop right now 馃槄 ) |
I have rebased to latest main, hopefully that should fix the 404 |
Damnit pymsql:
|
total_slots
be maxint
-like instead of -1
Infinite pools: Make their `total_slots` be `inf` instead of `-1` (cherry picked from commit 96f7643)
Infinite pools: Make their `total_slots` be `inf` instead of `-1`
Should fix #14515.
I saw 3 ways of fixing it:
maxint
slots instead of-1
.slots_stats
._executable_task_instances_to_queued
to understand-1
pools, changing all code operating onpool_slots_free
andopen_slots
# # # # #As to why I chose 2:
9223372036854775807
slots here on my system ), plus it increases the overall complexity of that function which is close to the core of the scheduler, we might as well try to keep it as lean as possible.