Skip to content

Remove schedule downstream tasks after execution (aka "mini scheduler") - #43741

Merged
ashb merged 1 commit into
mainfrom
remove-mini-scheduler-after-task
Nov 6, 2024
Merged

ashb merged 1 commit into
mainfrom
remove-mini-scheduler-after-task

Conversation

@ashb

@ashb ashb commented Nov 6, 2024

Copy link
Copy Markdown
Member

This has been questionable how much benefit it actually had, but with the move towards task DB isolation in Airflow 3 we won't be able to keep this anymore (as we didn't when AIP-44 DB isolation was enabled), so lets remove it now.

@ashb ashb added the area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK label Nov 6, 2024
@ashb
ashb requested a review from kaxil November 6, 2024 12:54
@tirkarthi

Copy link
Copy Markdown
Contributor

Docs portion can also be removed.

- :ref:`config:scheduler__schedule_after_task_execution`
Should the Task supervisor process perform a "mini scheduler" to attempt to schedule more tasks of
the same DAG. Leaving this on will mean tasks in the same DAG execute quicker,
but might starve out other DAGs in some circumstances.

@kaxil kaxil left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doc needs to be removed too but otherwise lgtm

@ashb

ashb commented Nov 6, 2024

Copy link
Copy Markdown
Member Author

Docs portion can also be removed.

- :ref:`config:scheduler__schedule_after_task_execution`
Should the Task supervisor process perform a "mini scheduler" to attempt to schedule more tasks of
the same DAG. Leaving this on will mean tasks in the same DAG execute quicker,
but might starve out other DAGs in some circumstances.

Thanks - I was grepping based on config option, guess I missed some

This has been questionable how much benefit it actually had, but with the move
towards task DB isolation in Airflow 3 we won't be able to keep this anymore
(as we didn't when AIP-44 DB isolation was enabled), so lets remove it now.
@ashb
ashb force-pushed the remove-mini-scheduler-after-task branch from 14c20ea to 3598af5 Compare November 6, 2024 13:12
@ashb
ashb merged commit d41c859 into main Nov 6, 2024
@ashb
ashb deleted the remove-mini-scheduler-after-task branch November 6, 2024 15:29
@raphaelauv

Copy link
Copy Markdown
Contributor

In multiple context of high number of concurrent task ( more than 1000 and around 50 new task by sec ) we had problems and by disabling the mini scheduler things are running way more smoothly,

maybe we could deprecate the option in 2.10.4 and set it to false in 2.11.0 , wdyt ?

@kaxil

kaxil commented Nov 6, 2024

Copy link
Copy Markdown
Member

In multiple context of high number of concurrent task ( more than 1000 and around 50 new task by sec ) we had problems and by disabling the mini scheduler things are running way more smoothly,

maybe we could deprecate the option in 2.10.4 and set it to false in 2.11.0 , wdyt ?

Yeah good point, fancy a PR to 2-10-test?

@jscheffl

jscheffl commented Nov 6, 2024

Copy link
Copy Markdown
Contributor

In multiple context of high number of concurrent task ( more than 1000 and around 50 new task by sec ) we had problems and by disabling the mini scheduler things are running way more smoothly,
maybe we could deprecate the option in 2.10.4 and set it to false in 2.11.0 , wdyt ?

Yeah good point, fancy a PR to 2-10-test?

I think deprecating the option as warning would be good. I also see it needs a re-work as with AIP-72 the function can not be "distributed on worker" as in the past.

Nevertheless when scheduling MANY tasks this was really a performance boost comparing Airflow v1 to Airflow v2 when this was added. Waiting for scheduler loop to schedule the next is really slowing down DAGs... so maybe we need a (future) similar mechanism to schedule next tasks immediately after one has finished to have the same low-latency like in the past... on the backend of course... (like a notification queue where it makes most-sense to schedule next because one task just finished...)

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
…") (apache#43741)

This has been questionable how much benefit it actually had, but with the move
towards task DB isolation in Airflow 3 we won't be able to keep this anymore
(as we didn't when AIP-44 DB isolation was enabled), so lets remove it now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers area:scheduler area:serialization area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

7 participants