You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Pei-Cheng-Yu (via GitHub)" <gi...@apache.org> on 2026/01/17 16:48:23 UTC
[PR] add bulk delete for dagruns [airflow]
Pei-Cheng-Yu opened a new pull request, #60718:
URL: https://github.com/apache/airflow/pull/60718
## Why
related #52439
## How
- add `BulkDeleteRunsButton` and `useBulkDeleteDagRuns` for collectively delete tasks
## video
https://github.com/user-attachments/assets/5d5578f5-ce93-49ff-9527-f597f807674c
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "Pei-Cheng-Yu (via GitHub)" <gi...@apache.org>.
Pei-Cheng-Yu commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3795654851
> Can you take a look at the `DeleteVariablesButton` and `DeleteConnectionsButton` and implement the same for TI.
Thanks for reviewing, I just updated 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #60718:
URL: https://github.com/apache/airflow/pull/60718#discussion_r3066502704
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
@@ -0,0 +1,101 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Button, Flex, Heading, Text, useDisclosure, VStack } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiTrash2 } from "react-icons/fi";
+
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { useBulkDeleteDagRuns, type SelectedRun } from "src/queries/useBulkDeleteDagRuns";
+
+type BulkDeleteRunsButtonProps = {
+ readonly clearSelections: VoidFunction;
+ readonly selectedRuns: Array<SelectedRun>;
+};
+
+const BulkDeleteRunsButton = ({ clearSelections, selectedRuns }: BulkDeleteRunsButtonProps) => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const { t: translate } = useTranslation();
+ const count = selectedRuns.length;
+
+ const { error, isPending, mutate } = useBulkDeleteDagRuns({
+ clearSelections,
+ onSuccessConfirm: onClose,
+ });
+
+ return (
+ <>
+ <Button
+ aria-label={translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ colorPalette="danger"
+ disabled={count === 0}
+ onClick={onOpen}
+ size="sm"
+ variant="outline"
+ >
+ <FiTrash2 />
+ {translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ </Button>
+
+ <Dialog.Root onOpenChange={onClose} open={open} size="xl">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <VStack align="start" gap={4}>
+ <Heading size="xl">
+ {translate("dags:runAndTaskActions.delete.dialog.title", {
+ type: translate("dagRun_other"),
+ })}
+ </Heading>
+ </VStack>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body width="full">
+ <Text color="fg" fontSize="md" fontWeight="semibold" mb={4}>
+ {translate("dags:runAndTaskActions.delete.dialog.warning", {
+ type: translate("dagRun_other"),
+ })}{" "}
+ ({count} {translate("dagRun", { count })})
Review 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.
```suggestion
({count} {count === 1 ? translate("dagRun_one") : translate("dagRun_other")})
```
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string): Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
+};
+
+type SelectionConfig = {
+ allRowsSelected: boolean;
+ onRowSelect: (key: string, selected: boolean) => void;
+ onSelectAll: (selected: boolean) => void;
+ selectedRows: Map<string, boolean>;
+};
+
+const runColumns = (
+ translate: TFunction,
+ dagId?: string,
+ selection?: SelectionConfig,
+): Array<ColumnDef<DAGRunResponse>> => [
+ ...(selection
+ ? [
+ {
+ 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>,
+ ]
Review 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`.
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+type Props = {
+ readonly clearSelections?: VoidFunction;
+ readonly onSuccessConfirm?: VoidFunction;
+};
+
+export const useBulkDeleteDagRuns = ({ clearSelections, onSuccessConfirm }: Props = {}) => {
+ const { t: translate } = useTranslation();
+ const queryClient = useQueryClient();
+ const deleteMutation = useDagRunServiceDeleteDagRun();
+ const [error, setError] = useState<unknown>(undefined);
+
+ 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",
Review 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.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string): Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
Review 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).
```suggestion
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: "" };
}
```
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+type Props = {
+ readonly clearSelections?: VoidFunction;
+ readonly onSuccessConfirm?: VoidFunction;
+};
+
+export const useBulkDeleteDagRuns = ({ clearSelections, onSuccessConfirm }: Props = {}) => {
+ const { t: translate } = useTranslation();
+ const queryClient = useQueryClient();
+ const deleteMutation = useDagRunServiceDeleteDagRun();
+ const [error, setError] = useState<unknown>(undefined);
+
+ 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,
Review 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.
```suggestion
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,
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-4323634720
@Pei-Cheng-Yu This PR has had no commits for 83 days, with 7 unresolved review threads from @pierrejeambrun still pending. Closing to keep the queue clean.
You are welcome to reopen this PR when you're ready to address the outstanding review feedback, or to open a new PR continuing this work. If you reopen, please rebase onto the current `main` first.
---
_Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "Pei-Cheng-Yu (via GitHub)" <gi...@apache.org>.
Pei-Cheng-Yu commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3938280880
Thanks a lot for the understanding and support!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "Pei-Cheng-Yu (via GitHub)" <gi...@apache.org>.
Pei-Cheng-Yu commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3834111957
Thanks for the reviews !
Sure! I just make the hook's return value align with the pattern in `useBulkDeleteVariables.ts`, and now i can investigate into the backend part!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #60718: add bulk delete for dagruns
URL: https://github.com/apache/airflow/pull/60718
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "Pei-Cheng-Yu (via GitHub)" <gi...@apache.org>.
Pei-Cheng-Yu commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3937712375
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!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-4292935654
@pierrejeambrun — Could you please check whether your review feedback on this PR has been addressed? @Pei-Cheng-Yu appears to have responded to your comments. @Pei-Cheng-Yu, do you believe the reviewer's concerns have been resolved?
If the concerns are resolved, please resolve the conversation threads. Thank you!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3835745029
Nice! Feel free to open a new pr to handle the backend part~
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #60718:
URL: https://github.com/apache/airflow/pull/60718#discussion_r3051608113
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
Review Comment:
nit: Rename file + component to `DeleteRunsButton` to be consistant with other `DeleteVariablesButton` and `
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string): Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
+};
+
+type SelectionConfig = {
+ allRowsSelected: boolean;
+ onRowSelect: (key: string, selected: boolean) => void;
+ onSelectAll: (selected: boolean) => void;
+ selectedRows: Map<string, boolean>;
+};
Review Comment:
This is alreaady defined in `useRowSelection.ts` we should reuse it.
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
@@ -0,0 +1,101 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Button, Flex, Heading, Text, useDisclosure, VStack } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiTrash2 } from "react-icons/fi";
+
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { useBulkDeleteDagRuns, type SelectedRun } from "src/queries/useBulkDeleteDagRuns";
+
+type BulkDeleteRunsButtonProps = {
+ readonly clearSelections: VoidFunction;
+ readonly selectedRuns: Array<SelectedRun>;
+};
+
+const BulkDeleteRunsButton = ({ clearSelections, selectedRuns }: BulkDeleteRunsButtonProps) => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const { t: translate } = useTranslation();
+ const count = selectedRuns.length;
+
+ const { error, isPending, mutate } = useBulkDeleteDagRuns({
+ clearSelections,
+ onSuccessConfirm: onClose,
+ });
+
+ return (
+ <>
+ <Button
+ aria-label={translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ colorPalette="danger"
+ disabled={count === 0}
+ onClick={onOpen}
+ size="sm"
+ variant="outline"
+ >
+ <FiTrash2 />
+ {translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ </Button>
Review Comment:
Not consistent. Here it's just delete:
<img width="444" height="196" alt="Image" src="https://github.com/user-attachments/assets/1a4bc19c-70ab-4b91-b167-75fb14bd16c8" />
<img width="589" height="181" alt="Image" src="https://github.com/user-attachments/assets/6d36b14c-e798-4353-9b23-0560a8123a3f" />
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -236,12 +292,33 @@ export const DagRuns = () => {
query.state.data?.dag_runs.some((run) => isStatePending(run.state)) ? refetchInterval : false,
},
);
+ const rows = data?.dag_runs ?? [];
- const columns = runColumns(translate, dagId);
+ const { allRowsSelected, clearSelections, handleRowSelect, handleSelectAll, selectedRows } =
+ useRowSelection({
+ data: rows,
+ getKey: (run) => runKey({ dagId: run.dag_id, dagRunId: run.dag_run_id }),
+ });
+
+ const selectedRuns = useMemo<Array<SelectedRun>>(
Review Comment:
React compiler is taking care of `useMemo` I belive. This isn't needed anymore.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +67,54 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string): Array<ColumnDef<DAGRunResponse>> => [
+const runKey = (run: SelectedRun) => `${run.dagId}__${run.dagRunId}`;
+const parseRunKey = (key: string): SelectedRun => {
+ const [dagId = "", ...rest] = key.split("__");
+
+ return { dagId, dagRunId: rest.join("__") };
+};
Review Comment:
What if the dag_id / dag_run_id holds a `__` ?
##########
airflow-core/src/airflow/ui/src/pages/BulkDeleteRunsButton.tsx:
##########
@@ -0,0 +1,101 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Button, Flex, Heading, Text, useDisclosure, VStack } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiTrash2 } from "react-icons/fi";
+
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { useBulkDeleteDagRuns, type SelectedRun } from "src/queries/useBulkDeleteDagRuns";
+
+type BulkDeleteRunsButtonProps = {
+ readonly clearSelections: VoidFunction;
+ readonly selectedRuns: Array<SelectedRun>;
+};
+
+const BulkDeleteRunsButton = ({ clearSelections, selectedRuns }: BulkDeleteRunsButtonProps) => {
+ const { onClose, onOpen, open } = useDisclosure();
+ const { t: translate } = useTranslation();
+ const count = selectedRuns.length;
+
+ const { error, isPending, mutate } = useBulkDeleteDagRuns({
+ clearSelections,
+ onSuccessConfirm: onClose,
+ });
+
+ return (
+ <>
+ <Button
+ aria-label={translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ colorPalette="danger"
+ disabled={count === 0}
+ onClick={onOpen}
+ size="sm"
+ variant="outline"
+ >
+ <FiTrash2 />
+ {translate("dags:runAndTaskActions.delete.button", { type: translate("dagRun_other") })}
+ </Button>
+
+ <Dialog.Root onOpenChange={onClose} open={open} size="xl">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <VStack align="start" gap={4}>
+ <Heading size="xl">
+ {translate("dags:runAndTaskActions.delete.dialog.title", {
+ type: translate("dagRun_other"),
+ })}
+ </Heading>
+ </VStack>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body width="full">
+ <Text color="fg" fontSize="md" fontWeight="semibold" mb={4}>
+ {translate("dags:runAndTaskActions.delete.dialog.warning", {
+ type: translate("dagRun_other"),
+ })}{" "}
+ ({count} {translate("dagRun", { count })})
+ </Text>
+
+ <ErrorAlert error={error} />
+
+ <Flex justifyContent="end" mt={3}>
+ <Button
+ colorPalette="danger"
+ loading={isPending}
+ onClick={() => {
+ void mutate(selectedRuns);
+ }}
+ >
+ <FiTrash2 />{" "}
+ <Text as="span" fontWeight="bold">
+ {translate("common:delete")}
Review Comment:
The confirmation modal isn't consistent with other confirmation modal:
<img width="1282" height="611" alt="Image" src="https://github.com/user-attachments/assets/8bb89e67-0a3c-4df2-b8fe-0af0ec8e9e6e" />
<img width="1099" height="505" alt="Image" src="https://github.com/user-attachments/assets/d51f5e62-7116-426b-a291-4db378056c4f" />
<img width="1328" height="638" alt="Image" src="https://github.com/user-attachments/assets/332537c4-4ef8-4dce-9330-bfb0762f9481" />
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+type Props = {
+ readonly clearSelections?: VoidFunction;
+ readonly onSuccessConfirm?: VoidFunction;
+};
+
+export const useBulkDeleteDagRuns = ({ clearSelections, onSuccessConfirm }: Props = {}) => {
+ const { t: translate } = useTranslation();
+ const queryClient = useQueryClient();
+ const deleteMutation = useDagRunServiceDeleteDagRun();
+ const [error, setError] = useState<unknown>(undefined);
+
+ 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",
+ });
Review 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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3935227692
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!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3937954978
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!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-4292954956
Sorry for that @pierrejeambrun -> new comments are coming.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #60718:
URL: https://github.com/apache/airflow/pull/60718#discussion_r2732397958
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,89 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+export const useBulkDeleteDagRuns = (onSuccessConfirm?: () => void) => {
+ const { t: translate } = useTranslation();
+ const queryClient = useQueryClient();
+ const deleteMutation = useDagRunServiceDeleteDagRun();
+ const [error, setError] = useState<unknown>(undefined);
+ const bulkDelete = async (runs: Array<SelectedRun>): Promise<void> => {
Review Comment:
Here is not real bulk delete of dagrun. We don't have bulk dagrun endpoint now. We need to update backend to support this or we would need N queries for N dagruns.
##########
airflow-core/src/airflow/ui/src/pages/DagRuns.tsx:
##########
@@ -58,7 +66,55 @@ const {
TRIGGERING_USER_NAME_PATTERN: TRIGGERING_USER_NAME_PATTERN_PARAM,
}: SearchParamsKeysType = SearchParamsKeys;
-const runColumns = (translate: TFunction, dagId?: string): Array<ColumnDef<DAGRunResponse>> => [
+type SelectedRun = { dagId: string; dagRunId: string };
Review Comment:
This is duplicated, let use the one at `useBulkDeleteDagRuns.ts:32`
##########
airflow-core/src/airflow/ui/src/queries/useBulkDeleteDagRuns.ts:
##########
@@ -0,0 +1,89 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient } from "@tanstack/react-query";
+import { useState } from "react";
+import { useTranslation } from "react-i18next";
+
+import {
+ useDagRunServiceDeleteDagRun,
+ useDagRunServiceGetDagRunsKey,
+ UseDagRunServiceGetDagRunKeyFn,
+ useTaskInstanceServiceGetTaskInstancesKey,
+ useTaskInstanceServiceGetHitlDetailsKey,
+} from "openapi/queries";
+import { toaster } from "src/components/ui";
+
+export type SelectedRun = { dagId: string; dagRunId: string };
+
+export const useBulkDeleteDagRuns = (onSuccessConfirm?: () => void) => {
Review Comment:
nit: The hook is kind of inconsistent here. We could try follow the pattern in `useBulkDeleteVariables.ts:31` and `useBulkDeleteConnections.tsx:31`:
- Accept clearSelections as a parameter
- Return { error, isPending, mutate }
- Keep consistency across the codebase
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add bulk delete for dagruns [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #60718:
URL: https://github.com/apache/airflow/pull/60718#issuecomment-3805667874
> Can you take a look at the DeleteVariablesButton and DeleteConnectionsButton and implement the same for TI.
Maybe we could handle that as another pr since I think that one is more complicated.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org