Discard task state for a taskinstance when it is cleared - #72100
Discard task state for a taskinstance when it is cleared#72100amoghrajesh wants to merge 4 commits into
Conversation
| @@ -0,0 +1,39 @@ | |||
| Clearing a task now discards its task state store entries | |||
There was a problem hiding this comment.
| Clearing a task now discards its task state store entries | |
| Clearing a task now discards its task state store entries by default |
I think? "By default" hopefully implying it's changeable?
There was a problem hiding this comment.
yep, handled in b4937ef1b7
| Operators with durable execution are worth particular attention. Clearing a *failed* task never runs | ||
| ``on_kill``, so an external job that outlived its worker is still running, and discarding the stored |
There was a problem hiding this comment.
This feels like it could be a bug in some respects.
If the max number of retries for a durable task has failed, shouldn't some callback on the operator handle this automatically?
There was a problem hiding this comment.
Yep, it is and not specific to durable tasks.
on_kill() only fires for SIGTERM, or execution_timeout being exceeded. A task that fails by raising never calls it. So a durable task that exhausts its retries can leave the external job running with nothing tracking it!
Checked the current durable operators. Eight of nine put cancellation only in on_kill (Glue, Redshift Data, BigQuery, Snowflake, Livy, SparkSubmit, both Databricks ones), so none clean up on failure. KPO is the exception, and by a different route: its cleanup sits in a finally in execute_sync honouring on_finish_action, so it runs on any exit. ResumableJobMixin has no cleanup path of its own.
I'd keep it out of this PR since it's terminal-failure cleanup rather than clear semantics, but if a terminally failed durable task cancelled its job, there'd be nothing left running for a later clear to duplicate, and most of this caveat goes away. I'll open an issue for that.
There was a problem hiding this comment.
I'll let @vatsrahul1001 chime in, but imo we don't need a significant note for this one.
There was a problem hiding this comment.
My reasoning for significant was the old behaviour shipped in 3.3.1, so anyone who clears a task today and expects it to resume from the checkpoint gets a different result after upgrading, without changing anything in their Dag. I'll wait for rahul too
There was a problem hiding this comment.
I agree with Amogh here. It's a very simple code change but a possibly very big behaviour change.
| # Clearing means "run this again", so the next attempt starts over rather than resuming from | ||
| # progress recorded by the attempt the user just discarded. Only after the clear has | ||
| # succeeded, so a failed clear cannot take the task state with it. |
There was a problem hiding this comment.
Probably don't need the front end of this comment, pretty intuitive. Could keep the "only after clear has suceeded" part, still optional imo.
There was a problem hiding this comment.
Cool, shortened in b4937ef1b7
|
|
||
| def _clear_task_state_store_on_success(tis: Sequence[TI], session: Session) -> None: | ||
| """Discard task state store entries for each TI if clear_on_success is enabled.""" | ||
| if not conf.getboolean("state_store", "clear_on_success", fallback=False): |
There was a problem hiding this comment.
| if not conf.getboolean("state_store", "clear_on_success", fallback=False): | |
| if not conf.getboolean("state_store", "clear_on_success"): |
Don't need the fallback, defaults come from config already.
There was a problem hiding this comment.
Sounds good. removed in b4937ef1b7
Was generative AI tooling used to co-author this PR?
Summary
Clearing a task instance now discards its
task_state_storeentries, so the next attempt starts over instead of resuming from a checkpoint or reconnecting to an external job recorded by the attempt that was cleared.Clearing a task instance now discards its
task_state_storeentries, so the next attempt startsover. Retries are unaffected and still resume.
Why
A retry and a clear were treated identically. Both kept the task's state, both resumed. But they mean different things.
A retry happens because infrastructure failed. Nothing about the work changed, so carrying on from the checkpoint is exactly right, and that is what crash recovery is for. A clear happens because a human intervened, and the usual reason a human clears a task is that something did change: the code, the upstream data, a connection, a config value.
So the behaviour was tuned for the case where nothing changed, and then applied to the case where
something had.
What that cost in practice:
Fixing a bug and clearing left the bug's output in place. A task gets through files 1 to 6, hits bad data on the 7th, you fix the transform and clear. It resumed at 7. Files 1 to 6 still held output from the code you had just fixed, now silently mixed with the corrected work. The task went green, and nothing anywhere said otherwise.
Clearing a succeeded durable task did nothing at all. The operator read back the stored job id, saw the external job had already finished, and returned the stored result in a couple of seconds. This needs no unusual configuration, and there is no reading of "clear" under which doing nothing is what the user asked for.
The two failure modes are asymmetric, which is what decides the default rather than just moving the
problem elsewhere. Discarding when you wanted to resume costs repeated work you can watch happen.
Resuming when you wanted a fresh start produces wrong output you cannot see.
What changed
keep_task_stateonClearTaskInstancesBody, defaulting tofalsepost_clear_task_instances, inside the existingif not dry_run:block andafter
clear_task_instancessucceeds, so a preview discards nothing and a failed clear cannottake the task state with it
_clear_task_state_store_on_successrefactored into a shareddiscard_task_state_storehelper:two callers, two gates, one implementation
existing "Prevent rerun if task is running"
a task is treated the same as a retry"
When to tick the box
Two situations, both documented.
Nothing changed and you only want the task to carry on. Retries normally cover this, so you reach it when retries are exhausted.
An external job is still running. Most operators cancel theirs in
on_kill, so clearing a running task leaves nothing to reconnect to. But clearing a failed task never runson_kill, so a job that outlived its worker is still going, and discarding the stored id submits a second one. Same for operators configured to leave the job alive, such asKubernetesPodOperatorwithon_kill_action="keep_pod".Compatibility
This changes behaviour introduced in 3.3 and released in 3.3.1. Anyone relying on clear-to-resume needs
keep_task_state=true.A single default rather than per-operator behaviour is acceptable precisely because the user keeps an override: if Airflow decided silently per operator, a wrong guess would be unrecoverable, whereas a wrong default is one checkbox.
Tests
Trying to run a dag like this:
This dag showcases some data transformation and mimics a case where bad data came in as
N/Acontrolled by a variable value.So showing it:
First run:
Task state store contains this:
Set the variable rightly now:
airflow variables set handle_missing trueClear with the default:
Next run:
State store:
Now if I cleared by overriding the checkbox for another try (ie: keeping task state):
Observe the logs:
{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.