Conversation
238658d to
5d2e210
Compare
Thanks for reviewing, I just updated it! |
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts
Outdated
Show resolved
Hide resolved
I think maybe we could handle that in another separated pr since I think that one is more complicated. |
There was a problem hiding this comment.
I agree that we need another PR to update the backend to add the bulk delete feature similarly to variables and connections before we can merge this.
This PR is looking good to me, but we probably don't want to make N calls. (if someone tries to delete 500 dagruns this way, it's probably going to be a problem)
@Pei-Cheng-Yu Would you be interested in contributing that backend feature to enable this PR first?
|
Thanks for the reviews ! |
|
Nice! Feel free to open a new pr to handle the backend part~ |
|
Hi @Pei-Cheng-Yu, I think this would be a great feature to include in 3.2. Just wanted to check if you're still working on this? If you're tied up at the moment, I’m happy to take it over and help handle it. Thanks! |
|
Hi @guan404ming , thanks for checking and apologies for the delay. The past few weeks have been quite busy on my end, so I’ve been working on this incrementally. The backend implementation is now completed, and only the unit tests remain. I expect to have them finished within the next two days. Really appreciate the offer to help! |
|
No need to apologize at all! I completely understand how busy things can get. It’s great to hear that the backend implementation is finished. If you run into any blockers or have questions while wrapping things up, feel free to reach out anytime. Looking forward to the update! |
|
Thanks a lot for the understanding and support! |
jscheffl
left a comment
There was a problem hiding this comment.
For me this is looking good. Yes, at the moment this makes N calls to the backend but it is not possible to make more batch calls than the list has elements, so 500 is not possible. I think it is more important to have the feature then making it pretty.
Therefore I think we can also merge this PR and have a follow-up to improve with a batch backend. But leaving final merge to an UI expert.
Tested manaully besides code check and works well.
pierrejeambrun
left a comment
There was a problem hiding this comment.
I agree that we can deliver value incrementally. Having the feature 'non optimal' is still better than not having the feature at all.
We've done this in the past for other parts of the UI.
I'm happy to move forward with this and refactor it later when scalability becomes a problem.
A few suggestions though before we can move forward.
There was a problem hiding this comment.
nit: Rename file + component to DeleteRunsButton to be consistant with other DeleteVariablesButton and `
| > | ||
| <FiTrash2 />{" "} | ||
| <Text as="span" fontWeight="bold"> | ||
| {translate("common:delete")} |
| <FiTrash2 /> | ||
| {translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })} | ||
| </Button> |
| type SelectionConfig = { | ||
| allRowsSelected: boolean; | ||
| onRowSelect: (key: string, selected: boolean) => void; | ||
| onSelectAll: (selected: boolean) => void; | ||
| selectedRows: Map<string, boolean>; | ||
| }; |
There was a problem hiding this comment.
This is alreaady defined in useRowSelection.ts we should reuse it.
| const parseRunKey = (key: string): SelectedRun => { | ||
| const [dagId = "", ...rest] = key.split("__"); | ||
|
|
||
| return { dagId, dagRunId: rest.join("__") }; | ||
| }; |
There was a problem hiding this comment.
What if the dag_id / dag_run_id holds a __ ?
| getKey: (run) => runKey({ dagId: run.dag_id, dagRunId: run.dag_run_id }), | ||
| }); | ||
|
|
||
| const selectedRuns = useMemo<Array<SelectedRun>>( |
There was a problem hiding this comment.
React compiler is taking care of useMemo I belive. This isn't needed anymore.
| if (failed.length > 0) { | ||
| const [first] = failed; | ||
|
|
||
| if (first) { | ||
| setError(first.reason); | ||
| } | ||
| toaster.create({ | ||
| description: `${failed.length}/${runs.length} failed`, | ||
| title: translate("dags:runAndTaskActions.delete.error", { type: translate("dagRun_one") }), | ||
| type: "error", | ||
| }); |
There was a problem hiding this comment.
When multiple deletions fail, only failed[0].reason is stored in error state. The toast says ${failed.length}/${runs.length} failed but doesn't say which ones or why. Consider showing all failed run IDs in the error alert.
There was a problem hiding this comment.
Pull request overview
Adds a bulk-delete workflow to the DAG Runs listing, enabling users to select multiple runs and delete them in one action (addressing the “select many runs and collectively remove…” workflow referenced in #52439).
Changes:
- Introduces a
BulkDeleteRunsButtonconfirmation dialog for deleting selected DAG runs. - Adds
useBulkDeleteDagRunsto perform multi-run deletion and invalidate relevant React Query caches. - Updates
DagRunspage to support row selection via checkboxes and show an action bar when rows are selected.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts |
New hook implementing multi-run deletion and cache invalidation with success/error toasts. |
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx |
Adds checkbox-based row selection and an ActionBar entry point for bulk deletion. |
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx |
New UI button + dialog to confirm and execute bulk deletion. |
| {translate("dags:runAndTaskActions.delete.dialog.warning", { | ||
| type: translate("dagRun_other"), | ||
| })}{" "} | ||
| ({count} {translate("dagRun", { count })}) |
There was a problem hiding this comment.
The translation call translate("dagRun", { count }) is likely incorrect: in public/i18n/locales/*/common.json, dagRun is a nested object (e.g. for column labels), not a pluralizable string key. This will render as the key name (or [object Object]) instead of “Dag Run(s)”. Use the existing plural keys like dagRun_one / dagRun_other (or an existing pluralized resourceName pattern) for the count label.
| ({count} {translate("dagRun", { count })}) | |
| ({count} {count === 1 ? translate("dagRun_one") : translate("dagRun_other")}) |
| { | ||
| cell: ({ row: { original } }: DagRunRow) => { | ||
| const key = runKey({ dagId: original.dag_id, dagRunId: original.dag_run_id }); | ||
|
|
||
| return ( | ||
| <Checkbox | ||
| borderWidth={1} | ||
| checked={selection.selectedRows.get(key)} | ||
| colorPalette="brand" | ||
| onCheckedChange={(event) => selection.onRowSelect(key, Boolean(event.checked))} | ||
| /> | ||
| ); | ||
| }, | ||
| enableSorting: false, | ||
| header: () => ( | ||
| <Checkbox | ||
| borderWidth={1} | ||
| checked={selection.allRowsSelected} | ||
| colorPalette="brand" | ||
| onCheckedChange={(event) => selection.onSelectAll(Boolean(event.checked))} | ||
| /> | ||
| ), | ||
| id: "select", | ||
| meta: { skeletonWidth: 2 }, | ||
| } satisfies ColumnDef<DAGRunResponse>, | ||
| ] |
There was a problem hiding this comment.
The selection checkbox column is missing enableHiding: false (and uses id instead of the established accessorKey: "select" pattern). Since DataTable enables column hiding globally, users can hide this column and lose the ability to change selections. Consider matching the existing selection-column pattern used in e.g. pages/Connections/Connections.tsx and pages/Variables/Variables.tsx by setting accessorKey: "select", enableHiding: false, and enableSorting: false.
| const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`; | ||
| const parseRunKey = (key: string): SelectedRun => { | ||
| const [dagId = "", ...rest] = key.split("__"); | ||
|
|
||
| return { dagId, dagRunId: rest.join("__") }; |
There was a problem hiding this comment.
runKey/parseRunKey rely on a "__" delimiter to round-trip { dagId, dagRunId }. This breaks if dagId itself contains "__" (DAG IDs are user-defined strings), leading to deletes being issued against the wrong DAG. Prefer an encoding that can’t collide (e.g. JSON.stringify/JSON.parse, or base64/URL-encoding of each part with an unambiguous separator).
| const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`; | |
| const parseRunKey = (key: string): SelectedRun => { | |
| const [dagId = "", ...rest] = key.split("__"); | |
| return { dagId, dagRunId: rest.join("__") }; | |
| const runKey = (run: SelectedRun) => JSON.stringify({ dagId: run.dagId, dagRunId: run.dagRunId }); | |
| const parseRunKey = (key: string): SelectedRun => { | |
| try { | |
| const parsed = JSON.parse(key) as Partial<SelectedRun>; | |
| return { | |
| dagId: typeof parsed.dagId === "string" ? parsed.dagId : "", | |
| dagRunId: typeof parsed.dagRunId === "string" ? parsed.dagRunId : "", | |
| }; | |
| } catch { | |
| return { dagId: "", dagRunId: "" }; | |
| } |
|
|
||
| const mutate = async (runs: Array<SelectedRun>): Promise<void> => { | ||
| if (runs.length === 0) { | ||
| return; | ||
| } | ||
| setError(undefined); | ||
|
|
||
| const results = await Promise.allSettled( | ||
| runs.map(({ dagId, dagRunId }) => deleteMutation.mutateAsync({ dagId, dagRunId })), | ||
| ); | ||
|
|
||
| const failed = results.filter((result): result is PromiseRejectedResult => result.status === "rejected"); | ||
| const queryKeys = [ | ||
| [useDagRunServiceGetDagRunsKey], | ||
| [useTaskInstanceServiceGetTaskInstancesKey], | ||
| [useTaskInstanceServiceGetHitlDetailsKey], | ||
| ...runs.map(({ dagId, dagRunId }) => UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId })), | ||
| ]; | ||
|
|
||
| await Promise.all(queryKeys.map((key) => queryClient.invalidateQueries({ queryKey: key }))); | ||
|
|
||
| if (failed.length > 0) { | ||
| const [first] = failed; | ||
|
|
||
| if (first) { | ||
| setError(first.reason); | ||
| } | ||
| toaster.create({ | ||
| description: `${failed.length}/${runs.length} failed`, | ||
| title: translate("dags:runAndTaskActions.delete.error", { type: translate("dagRun_one") }), | ||
| type: "error", | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| toaster.create({ | ||
| description: translate("dags:runAndTaskActions.delete.success.description", { | ||
| type: translate("dagRun_one"), | ||
| }), | ||
| title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), | ||
| type: "success", | ||
| }); | ||
|
|
||
| clearSelections?.(); | ||
| onSuccessConfirm?.(); | ||
| }; | ||
|
|
||
| return { | ||
| error, | ||
| isPending: deleteMutation.isPending, |
There was a problem hiding this comment.
useDagRunServiceDeleteDagRun() returns a single React Query mutation instance, but mutateAsync is invoked concurrently for every selected run. Mutation state (notably isPending) only tracks the most recently started mutation, so the UI loading state can flip to false while earlier deletions are still in flight. Consider implementing this as one mutation that accepts the array of runs (and internally runs deletes with a concurrency limit or sequentially), and manage a dedicated isPending state for the bulk operation.
| const mutate = async (runs: Array<SelectedRun>): Promise<void> => { | |
| if (runs.length === 0) { | |
| return; | |
| } | |
| setError(undefined); | |
| const results = await Promise.allSettled( | |
| runs.map(({ dagId, dagRunId }) => deleteMutation.mutateAsync({ dagId, dagRunId })), | |
| ); | |
| const failed = results.filter((result): result is PromiseRejectedResult => result.status === "rejected"); | |
| const queryKeys = [ | |
| [useDagRunServiceGetDagRunsKey], | |
| [useTaskInstanceServiceGetTaskInstancesKey], | |
| [useTaskInstanceServiceGetHitlDetailsKey], | |
| ...runs.map(({ dagId, dagRunId }) => UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId })), | |
| ]; | |
| await Promise.all(queryKeys.map((key) => queryClient.invalidateQueries({ queryKey: key }))); | |
| if (failed.length > 0) { | |
| const [first] = failed; | |
| if (first) { | |
| setError(first.reason); | |
| } | |
| toaster.create({ | |
| description: `${failed.length}/${runs.length} failed`, | |
| title: translate("dags:runAndTaskActions.delete.error", { type: translate("dagRun_one") }), | |
| type: "error", | |
| }); | |
| return; | |
| } | |
| toaster.create({ | |
| description: translate("dags:runAndTaskActions.delete.success.description", { | |
| type: translate("dagRun_one"), | |
| }), | |
| title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), | |
| type: "success", | |
| }); | |
| clearSelections?.(); | |
| onSuccessConfirm?.(); | |
| }; | |
| return { | |
| error, | |
| isPending: deleteMutation.isPending, | |
| const [isPending, setIsPending] = useState(false); | |
| const mutate = async (runs: Array<SelectedRun>): Promise<void> => { | |
| if (runs.length === 0) { | |
| return; | |
| } | |
| setError(undefined); | |
| setIsPending(true); | |
| try { | |
| const failed: Array<PromiseRejectedResult> = []; | |
| for (const { dagId, dagRunId } of runs) { | |
| try { | |
| await deleteMutation.mutateAsync({ dagId, dagRunId }); | |
| } catch (reason) { | |
| failed.push({ reason, status: "rejected" }); | |
| } | |
| } | |
| const queryKeys = [ | |
| [useDagRunServiceGetDagRunsKey], | |
| [useTaskInstanceServiceGetTaskInstancesKey], | |
| [useTaskInstanceServiceGetHitlDetailsKey], | |
| ...runs.map(({ dagId, dagRunId }) => UseDagRunServiceGetDagRunKeyFn({ dagId, dagRunId })), | |
| ]; | |
| await Promise.all(queryKeys.map((key) => queryClient.invalidateQueries({ queryKey: key }))); | |
| if (failed.length > 0) { | |
| const [first] = failed; | |
| if (first) { | |
| setError(first.reason); | |
| } | |
| toaster.create({ | |
| description: `${failed.length}/${runs.length} failed`, | |
| title: translate("dags:runAndTaskActions.delete.error", { type: translate("dagRun_one") }), | |
| type: "error", | |
| }); | |
| return; | |
| } | |
| toaster.create({ | |
| description: translate("dags:runAndTaskActions.delete.success.description", { | |
| type: translate("dagRun_one"), | |
| }), | |
| title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), | |
| type: "success", | |
| }); | |
| clearSelections?.(); | |
| onSuccessConfirm?.(); | |
| } finally { | |
| setIsPending(false); | |
| } | |
| }; | |
| return { | |
| error, | |
| isPending, |
| toaster.create({ | ||
| description: `${failed.length}/${runs.length} failed`, | ||
| title: translate("dags:runAndTaskActions.delete.error", { type: translate("dagRun_one") }), | ||
| type: "error", | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| toaster.create({ | ||
| description: translate("dags:runAndTaskActions.delete.success.description", { | ||
| type: translate("dagRun_one"), | ||
| }), | ||
| title: translate("dags:runAndTaskActions.delete.success.title", { type: translate("dagRun_one") }), | ||
| type: "success", |
There was a problem hiding this comment.
The bulk-failure toast text is not localized ("${failed.length}/${runs.length} failed") and the toast titles/descriptions use dagRun_one even though multiple runs may be deleted. Please switch to existing i18n keys (e.g. common:toaster.bulkDelete.* or a new dags:* bulk-delete key) and use the plural resource name (dagRun_other) and/or include the counts so the message is accurate.







Why
related #52439
How
BulkDeleteRunsButtonanduseBulkDeleteDagRunsfor collectively delete runsvideo
Clipchamp.7.mp4
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.