bbovenzi commented on code in PR #62195:
URL: https://github.com/apache/airflow/pull/62195#discussion_r3119855735
##########
airflow-core/src/airflow/serialization/encoders.py:
##########
Review Comment:
Let's revert changes to this file
##########
airflow-core/src/airflow/ui/public/i18n/locales/en/dag.json:
##########
@@ -103,6 +133,16 @@
"assetEvent_one": "Created Asset Event",
"assetEvent_other": "Created Asset Events"
},
+ "deadlines": {
+ "completionRule": "Complete within {{interval}} of {{reference}}",
+ "finishedEarly": "Finished {{duration}} before deadline",
+ "finishedLate": "Finished {{duration}} after deadline",
+ "pending": "Pending Deadlines",
+ "recentlyMissed": "Recently Missed Deadlines",
+ "stillRunning": "Still running past deadline",
+ "title": "Deadlines",
+ "viewAll": "View all {{count}}"
Review Comment:
We have a few repeated strings here. I'd love to keep the number of new
translation keys to a minimum.
Also ones like two different `completionRule` that are not meaningfully
different.
##########
airflow-core/src/airflow/ui/src/pages/Dag/Overview/DeadlineRow.tsx:
##########
@@ -0,0 +1,102 @@
+/*!
+ * 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 { HStack, Link, Text, VStack } from "@chakra-ui/react";
+import dayjs from "dayjs";
+import duration from "dayjs/plugin/duration";
+import relativeTime from "dayjs/plugin/relativeTime";
+import { useTranslation } from "react-i18next";
+import { Link as RouterLink } from "react-router-dom";
+
+import type {
+ DAGRunResponse,
+ DeadlineAlertResponse,
+ DeadlineResponse,
+ DagRunState,
+} from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+
+dayjs.extend(duration);
+dayjs.extend(relativeTime);
+
+type DeadlineRowProps = {
+ readonly alert?: DeadlineAlertResponse;
+ readonly deadline: DeadlineResponse;
+ readonly run?: DAGRunResponse;
+ readonly runState?: DagRunState;
+};
+
+export const DeadlineRow = ({ alert, deadline, run, runState }:
DeadlineRowProps) => {
+ const { t: translate } = useTranslation("dag");
+
+ const reference = alert
+ ? translate(`deadlineAlerts.referenceType.${alert.reference_type}`, {
+ defaultValue: alert.reference_type,
+ })
+ : undefined;
+ const interval = alert ? dayjs.duration(alert.interval,
"seconds").humanize() : undefined;
+
+ let contextLine: string | undefined;
+
+ if (deadline.missed) {
+ if (run?.end_date === undefined || run.end_date === null) {
+ contextLine = translate("overview.deadlines.stillRunning");
+ } else {
+ const diff = dayjs(run.end_date).diff(dayjs(deadline.deadline_time));
+
+ contextLine =
+ diff >= 0
+ ? translate("overview.deadlines.finishedLate", {
+ duration: dayjs.duration(diff).humanize(),
+ })
+ : translate("overview.deadlines.finishedEarly", {
+ duration: dayjs.duration(-diff).humanize(),
+ });
+ }
+ }
+
+ return (
+ <HStack justifyContent="space-between" px={2} py={1.5} width="100%">
+ <VStack alignItems="flex-start" gap={0}>
+ <HStack gap={1}>
+ <Link asChild color="fg.info" fontSize="sm" fontWeight="bold">
+ <RouterLink
to={`/dags/${deadline.dag_id}/runs/${deadline.dag_run_id}`}>
+ {deadline.dag_run_id}
+ </RouterLink>
+ </Link>
+ {deadline.missed && runState !== undefined ? (
+ <Text fontSize="xs" textTransform="capitalize">
+ ({runState})
+ </Text>
Review Comment:
We should use StateBadge here
##########
airflow-core/src/airflow/ui/src/pages/Dag/Overview/DeadlineRow.tsx:
##########
@@ -0,0 +1,102 @@
+/*!
+ * 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 { HStack, Link, Text, VStack } from "@chakra-ui/react";
+import dayjs from "dayjs";
+import duration from "dayjs/plugin/duration";
+import relativeTime from "dayjs/plugin/relativeTime";
+import { useTranslation } from "react-i18next";
+import { Link as RouterLink } from "react-router-dom";
+
+import type {
+ DAGRunResponse,
+ DeadlineAlertResponse,
+ DeadlineResponse,
+ DagRunState,
+} from "openapi/requests/types.gen";
+import Time from "src/components/Time";
+
+dayjs.extend(duration);
+dayjs.extend(relativeTime);
+
+type DeadlineRowProps = {
+ readonly alert?: DeadlineAlertResponse;
+ readonly deadline: DeadlineResponse;
+ readonly run?: DAGRunResponse;
+ readonly runState?: DagRunState;
+};
+
+export const DeadlineRow = ({ alert, deadline, run, runState }:
DeadlineRowProps) => {
+ const { t: translate } = useTranslation("dag");
+
+ const reference = alert
+ ? translate(`deadlineAlerts.referenceType.${alert.reference_type}`, {
+ defaultValue: alert.reference_type,
+ })
+ : undefined;
+ const interval = alert ? dayjs.duration(alert.interval,
"seconds").humanize() : undefined;
+
+ let contextLine: string | undefined;
+
+ if (deadline.missed) {
+ if (run?.end_date === undefined || run.end_date === null) {
+ contextLine = translate("overview.deadlines.stillRunning");
+ } else {
+ const diff = dayjs(run.end_date).diff(dayjs(deadline.deadline_time));
+
+ contextLine =
+ diff >= 0
+ ? translate("overview.deadlines.finishedLate", {
+ duration: dayjs.duration(diff).humanize(),
+ })
+ : translate("overview.deadlines.finishedEarly", {
+ duration: dayjs.duration(-diff).humanize(),
+ });
+ }
+ }
+
+ return (
+ <HStack justifyContent="space-between" px={2} py={1.5} width="100%">
+ <VStack alignItems="flex-start" gap={0}>
+ <HStack gap={1}>
+ <Link asChild color="fg.info" fontSize="sm" fontWeight="bold">
+ <RouterLink
to={`/dags/${deadline.dag_id}/runs/${deadline.dag_run_id}`}>
+ {deadline.dag_run_id}
+ </RouterLink>
+ </Link>
+ {deadline.missed && runState !== undefined ? (
+ <Text fontSize="xs" textTransform="capitalize">
+ ({runState})
+ </Text>
+ ) : undefined}
+ </HStack>
+ {reference !== undefined && interval !== undefined ? (
+ <Text color="fg.muted" fontSize="xs">
+ {translate("overview.deadlines.completionRule", { interval,
reference })}
+ </Text>
+ ) : undefined}
+ {contextLine !== undefined && (
+ <Text color="fg.error" fontSize="xs">
+ {contextLine}
+ </Text>
+ )}
+ </VStack>
+ <Time datetime={deadline.deadline_time} fontSize="sm" />
Review Comment:
I think that we are showing the deadline_time out of context. Maybe instead
of the humanize "FInished 6 minutes late" we can have a column of "Expected:
{deadline_time} , Actual: {"still running" or end_date (6min)}"
##########
airflow-core/src/airflow/ui/src/pages/Dag/Overview/AllDeadlinesModal.tsx:
##########
@@ -0,0 +1,156 @@
+/*!
+ * 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 { Heading, HStack, Separator, Skeleton, VStack } from
"@chakra-ui/react";
+import { useState } from "react";
+
+import { useDagRunServiceGetDagRuns, useDeadlinesServiceGetDeadlines } from
"openapi/queries";
+import type { DAGRunResponse, DagRunState, DeadlineAlertResponse } from
"openapi/requests/types.gen";
+import { ErrorAlert } from "src/components/ErrorAlert";
+import { Dialog } from "src/components/ui";
+import { Pagination } from "src/components/ui/Pagination";
+
+import { DeadlineRow } from "./DeadlineRow";
+
+const PAGE_LIMIT = 10;
+
+type AllDeadlinesModalProps = {
+ readonly alertMap: Map<string, DeadlineAlertResponse>;
+ readonly dagId: string;
+ readonly endDate: string;
+ readonly missed: boolean;
+ readonly onClose: () => void;
+ readonly open: boolean;
+ readonly refetchInterval: number | false;
+ readonly startDate: string;
+ readonly title: string;
+};
+
+export const AllDeadlinesModal = ({
+ alertMap,
+ dagId,
+ endDate,
+ missed,
+ onClose,
+ open,
+ refetchInterval,
+ startDate,
+ title,
+}: AllDeadlinesModalProps) => {
+ const [page, setPage] = useState(1);
+ const offset = (page - 1) * PAGE_LIMIT;
+
+ const { data, error, isLoading } = useDeadlinesServiceGetDeadlines(
+ missed
+ ? {
+ dagId,
+ dagRunId: "~",
+ lastUpdatedAtGte: startDate,
+ lastUpdatedAtLte: endDate,
+ limit: PAGE_LIMIT,
+ missed: true,
+ offset,
+ orderBy: ["-last_updated_at"],
+ }
+ : {
+ dagId,
+ dagRunId: "~",
+ deadlineTimeGte: endDate,
+ limit: PAGE_LIMIT,
+ missed: false,
+ offset,
+ orderBy: ["deadline_time"],
+ },
+ undefined,
+ { enabled: open, refetchInterval },
+ );
+
+ const { data: runsData } = useDagRunServiceGetDagRuns(
+ { dagId, limit: 100, runAfterGte: startDate, runAfterLte: endDate },
+ undefined,
+ { enabled: open, refetchInterval },
+ );
+
+ const runStateMap = new Map<string, DagRunState>();
+ const runMap = new Map<string, DAGRunResponse>();
+
+ for (const run of runsData?.dag_runs ?? []) {
+ runStateMap.set(run.dag_run_id, run.state);
+ runMap.set(run.dag_run_id, run);
+ }
+
+ const deadlines = data?.deadlines ?? [];
+ const totalEntries = data?.total_entries ?? 0;
+
+ const getAlert = (alertId?: string | null) =>
+ alertId !== undefined && alertId !== null ? alertMap.get(alertId) :
undefined;
+
+ const onOpenChange = () => {
+ setPage(1);
+ onClose();
+ };
+
+ return (
+ <Dialog.Root onOpenChange={onOpenChange} open={open}
scrollBehavior="inside" size="lg">
+ <Dialog.Content backdrop p={4}>
+ <Dialog.Header>
+ <Heading size="sm">{title}</Heading>
+ </Dialog.Header>
+ <Dialog.CloseTrigger />
+ <Dialog.Body pb={2}>
+ <ErrorAlert error={error} />
+ {isLoading ? (
+ <VStack>
+ {Array.from({ length: PAGE_LIMIT }).map((_, idx) => (
+ // eslint-disable-next-line react/no-array-index-key
+ <Skeleton height="36px" key={idx} width="100%" />
+ ))}
+ </VStack>
+ ) : (
+ <VStack gap={0} separator={<Separator />}>
+ {deadlines.map((dl) => (
+ <DeadlineRow
+ alert={getAlert(dl.alert_id)}
+ deadline={dl}
+ key={dl.id}
+ run={runMap.get(dl.dag_run_id)}
+ runState={runStateMap.get(dl.dag_run_id)}
Review Comment:
If we have the run then we don't need to pass the run state separately
--
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]
