Skip to content

Can end task instance from trigger#31808

Closed
dstandish wants to merge 16 commits into
apache:mainfrom
astronomer:trigger-can-exit-task
Closed

Can end task instance from trigger#31808
dstandish wants to merge 16 commits into
apache:mainfrom
astronomer:trigger-can-exit-task

Conversation

@dstandish

@dstandish dstandish commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

With this change, if desired, we can avoid having to resume the task on a worker. We can end the task (in states success, failed, or skipped) from the trigger itself.

Closes: #31718

cc @Lee-W

@dstandish dstandish requested a review from uranusjr as a code owner June 8, 2023 21:23
@dstandish dstandish marked this pull request as draft June 8, 2023 21:24
@dstandish dstandish marked this pull request as ready for review June 8, 2023 22:01
Comment thread airflow/triggers/temporal.py Outdated
Comment thread airflow/models/trigger.py Outdated
Comment thread airflow/sensors/time_delta.py Outdated
Comment thread airflow/triggers/base.py Outdated
Comment thread airflow/exceptions.py Outdated
Comment thread airflow/exceptions.py Outdated
@eladkal eladkal added this to the Airflow 2.7.0 milestone Jun 9, 2023
@eladkal eladkal added the type:improvement Changelog: Improvements label Jun 9, 2023
With this change, if desired, we can avoid having to resume the task on a worker.  We can end the task (in states success, failed, or skipped) from the trigger itself.

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@dstandish dstandish force-pushed the trigger-can-exit-task branch from a48edbb to d62624d Compare June 10, 2023 06:49
@dstandish dstandish changed the title Can exit task from trigger Can end task instance from trigger Jun 10, 2023
Comment thread airflow/models/taskinstance.py Outdated
Comment thread airflow/triggers/base.py Outdated
Comment thread airflow/triggers/base.py Outdated
Comment on lines +129 to +131
def __init__(self, *, xcom_return: Any | None = None, other_xcom: dict[str, Any] | None = None, **kwargs):
if "payload" in kwargs:
raise ValueError("Param 'payload' not supported for this class.")

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.

Why is this needed? I imagine this should work:

Suggested change
def __init__(self, *, xcom_return: Any | None = None, other_xcom: dict[str, Any] | None = None, **kwargs):
if "payload" in kwargs:
raise ValueError("Param 'payload' not supported for this class.")
def __init__(self, *, xcom_return: Any | None = None, other_xcom: dict[str, Any] | None = None) -> None:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the payload is determined by the state / controlled by the class. User should not submit a payload.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we do not check, user might pass payload and it would be silently ignored

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.

I don’t think so? The suggested change removed payload altogether and should raise TypeError when it is provided. Or is the issue you want to allow **kwargs? But I don’t see any subclass using that either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah i see what you're saying... Well it's in the base class. Would you not say it's probably a good idea to include kwargs in the subclasses for future compat?

Comment thread airflow/triggers/temporal.py Outdated
Comment thread airflow/triggers/temporal.py Outdated
@dstandish

Copy link
Copy Markdown
Contributor Author

ok made all changes except checking for user-supplied 'payload'

@dstandish

Copy link
Copy Markdown
Contributor Author

marking this draft for now because we have to deal with callbacks and email

@dstandish dstandish marked this pull request as draft June 14, 2023 18:19
@github-actions

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 30, 2023
@github-actions github-actions Bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 4, 2023
@dstandish dstandish removed this from the Airflow 2.8.0 milestone Nov 22, 2023
@github-actions

github-actions Bot commented Jan 7, 2024

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 7, 2024
@github-actions github-actions Bot closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add capability to exit tasks from trigger

5 participants