Repository navigation
Conversation
potiuk
added
the
ready for maintainer review
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — good rename, and it closes out the TODO that was sitting right there in client.py ("finish sounds wrong as 'even' deferred is essentially finishing"). set_terminal_state says what the method does; finish implied it handled every terminal transition when the server endpoint explicitly refuses SUCCESS / DEFERRED / SERVER_TERMINATED.
I checked the blast radius before agreeing this is safe to rename outright:
TaskInstanceOperationsisn't exported fromairflow.sdk— it's internal execution-time plumbing, not something Dag authors call, so no deprecation alias is warranted. Keeping one would just preserve the confusing name.- The only caller in the tree is
supervisor.py, updated here. - No wire-protocol change — the endpoint and payload are untouched, only the client method name and comments, so this doesn't interact with Execution API versioning.
The rewritten comments in supervisor.py are a real improvement too: "the generic non-success terminal-state update" is much clearer about why it can't substitute for the direct calls than "finish()" was.
One thing worth double-checking inline.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
|
Also that would need @amoghrajesh @ashb or @kaxil review. |
ff728dc to
54b3053
Compare
potiuk
added
the
closed because of open PR limit
|
Hello @fat-catTW - thank you for your contributions to Apache Airflow! The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 17 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are. What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack. Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
Why
The Task SDK client previously exposed this operation as
finish(), but that name made it sound like the generic path for any task that stops executing. In practice, this method only reports non-success terminal Task Instance states; success, retry, defer, reschedule, and awaiting-input transitions use dedicated methods and payloads.Change
Rename the Task SDK client method used for non-success terminal Task Instance state updates from
finish()toset_terminal_state().This better reflects that success, retry, defer, reschedule, and awaiting-input states each use dedicated API payloads and should not fall back to this generic non-success terminal-state update.
Updated the supervisor call sites, comments, unit tests, and Task SDK integration test usage.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.