kaxil commented on code in PR #72100:
URL: https://github.com/apache/airflow/pull/72100#discussion_r4050863910
##########
airflow-core/src/airflow/ui/src/components/Clear/TaskInstance/ClearGroupTaskInstanceDialog.tsx:
##########
@@ -53,22 +53,37 @@ export const ClearGroupTaskInstanceDialog = ({ onClose,
open, taskInstance }: Pr
const { dagId = "", runId = "" } = useParams();
const groupId = taskInstance.task_id;
- const { isPending, mutate } = useClearTaskInstances({
- dagId,
- dagRunId: runId,
- onSuccessConfirm: onClose,
- });
-
const [clearTaskInstanceDefaultOptions] =
useClearTaskInstanceDefaultOptions();
+ const [keepTaskStateDefault] = useClearKeepTaskStateDefault();
const [selectedOptions, setSelectedOptions] =
useState<Array<string>>(clearTaskInstanceDefaultOptions);
const onlyFailed = selectedOptions.includes("onlyFailed");
const past = selectedOptions.includes("past");
const future = selectedOptions.includes("future");
const upstream = selectedOptions.includes("upstream");
const downstream = selectedOptions.includes("downstream");
+ const [keepTaskState, setKeepTaskState] = useState(keepTaskStateDefault);
const [note, setNote] = useState<string | null>(null);
+ const onCloseDialog = () => {
+ setNote(null);
+ setKeepTaskState(keepTaskStateDefault);
+ onClose();
+ };
+
+ useEffect(() => {
+ if (open) {
+ setNote(null);
+ setKeepTaskState(false);
Review Comment:
`setKeepTaskState(false)` here ignores the preference this PR just added.
`keepTaskStateDefault` is read at `:57`, seeds the state at `:65`, and
`onCloseDialog` resets to it at `:70`, so three writes in this one component
disagree and the effect is the odd one out. The sibling gets it right:
`ClearTaskInstanceDialog.tsx:106` sets `keepTaskStateDefault`, with the value
in the dep array at `:108`.
The effect wins on every open, including the first.
`ClearTaskInstanceButton.tsx:98` renders this dialog gated on
`useInternalDialog && isGroup`, not on `open`, and `useDisclosure()` at `:60`
starts `false`, so the component mounts while closed, the `:65` initialiser
runs at that point, and the first `open` transition overwrites it. That leaves
`:65` dead on every reachable path.
A user who turns on "Keep task state on clear" in Settings therefore gets it
honoured in the single-task dialog and in the bulk clear, and silently ignored
when clearing a task group: the box is unchecked, `keep_task_state` is omitted
from the request at `:173`, the server default `False` applies, and the group's
entries are discarded. A task group is the widest blast radius of the three
surfaces.
This is not the stale-checkbox reset from the earlier rounds, which was
about the tick persisting across opens. That fix is what introduced this
effect, and it landed with the wrong value in one of the three.
`setKeepTaskState(keepTaskStateDefault)` at `:77` plus
`keepTaskStateDefault` in the deps at `:79` matches the sibling. The `[open]`
array is exhaustive as written, which is why the linter stayed quiet: writing
the correct value is what forces the dep change. There is also no test file for
any of the three clear dialogs, so nothing would have caught this.
##########
airflow-core/newsfragments/72100.significant.rst:
##########
@@ -0,0 +1,57 @@
+Clearing a task now discards its task state store entries by default
+
+Clearing a task instance discards its ``task_state_store`` entries, so the
next attempt starts from
+the beginning instead of resuming from a checkpoint or reconnecting to an
external job recorded by
+the attempt that was cleared.
+
+Retries are unaffected. They keep task state exactly as before, which is what
crash recovery relies
+on. Only a deliberate clear discards.
+
+This only applies to clearing individual task instances (the task-instance
clear endpoint / dialog,
+and ``airflowctl dags clear``, which clears every task instance in the matched
Dag run(s) through
+the same endpoint). Clearing an entire Dag run through the "Clear Run"
dialog/API, and marking a
+task as failed or success (which clears downstream tasks as a side effect),
still keep task state
+unconditionally today; extending discard-by-default to those paths is tracked
in
+`#72929 <https://github.com/apache/airflow/issues/72929>`_.
+
+**Why**
+
+Clearing means "run this again". A checkpoint records how far a task got, not
what it got there
+with, so resuming after the code or the upstream data changed left work done
before the fix in place
+and silently mixed it with the corrected work. Clearing a task whose external
job had already
+succeeded was worse: the operator read the stored result back and returned in
seconds having run
+nothing.
+
+**Keeping the old behaviour**
+
+Pass ``keep_task_state=True`` to the clear task instances endpoint, or tick
"keep task state" in the
Review Comment:
The opt-out described here stops at the request field and the dialog tick,
but this PR also ships a persisted default: Settings > Clearing > "Keep task
state on clear" (`Settings.tsx:276-286`), stored in localStorage via
`useClearKeepTaskStateDefault` (`hooks/useUserSettings.ts:61`) and seeded into
all three clear dialogs. As written, a reader of the release notes concludes
the only way to opt out is to tick the box on every clear.
That matters beyond completeness, because the preference changes what the
dialog shows by default: two people on the same deployment can open the same
clear dialog and see different initial states, with nothing in the docs
explaining why or where to change it. One sentence pointing at the Settings row
would cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
