Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECS Executor: Set tasks to RUNNING state once active #39212

Merged
merged 6 commits into from
May 6, 2024

Conversation

o-nikolas
Copy link
Contributor

Tasks were previously being put into QUEUED state after they were active in the ECS executor. This was to store executor state for task adoption but had the side effect of removing them from the list of running task instances (which has other knock-on effects). Instead change tasks into the RUNNING state, and do not remove them from the list of running tasks.

Also added some test coverage for the new and existing state change functions on the base executor class


^ 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.

Tasks were previously being put into QUEUED state after they were active
in the ECS executor. This was to store executor state for task adoption
but had the side effect of removing them from the list of running task
instances (which has other knock-on effects). Instead change tasks into
the RUNNING state, and do not remove them from the list of running
tasks.
@boring-cyborg boring-cyborg bot added area:Executors-core LocalExecutor & SequentialExecutor area:providers provider:amazon-aws AWS/Amazon - related issues labels Apr 24, 2024
@o-nikolas
Copy link
Contributor Author

CC @ferruzzi @syedahsn

Copy link
Contributor

@syedahsn syedahsn left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work on the added tests

- DebugExecutor: was overriding the change_state method from the base
executor, but changing no behaviour, so move to using the base executor
implementation
- CeleryExecutor: Plumb through the new param so that the signature
matches the base executor
o-nikolas added 2 commits May 1, 2024 15:55
…state

This allows task adoption to still work while using a more accurate
state for the executor.
There is a task filter earlier in the event processing method that I
missed originally
@o-nikolas o-nikolas merged commit a74b5f0 into apache:main May 6, 2024
41 checks passed
@o-nikolas o-nikolas deleted the onikolas/ecs_running_task_state_fix branch May 6, 2024 17:29
pateash pushed a commit to pateash/airflow that referenced this pull request May 13, 2024
Tasks were previously being put into QUEUED state after they were active
in the ECS executor. This was to store executor state for task adoption
but had the side effect of removing them from the list of running task
instances (which has other knock-on effects). Instead, change tasks into
the RUNNING state, and do not remove them from the list of running
tasks.

* Update change_state usage in debug and celery executor

- DebugExecutor: was overriding the change_state method from the base
executor, but changing no behaviour, so move to using the base executor
implementation
- CeleryExecutor: Plumb through the new param so that the signature
matches the base executor

* Call running_state in try/catch for backcompat
@eladkal eladkal added this to the Airflow 2.9.2 milestone May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Executors-core LocalExecutor & SequentialExecutor area:providers provider:amazon-aws AWS/Amazon - related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants