Conversation
|
Let us know when this is ready for review. (take your time, fix the CI, thorough local tests, etc..., there is no hurry) |
bbovenzi
left a comment
There was a problem hiding this comment.
Let's remove the Dashboard changes for now since they will really depend on the UX in your other PR and we can just focus on the browse page.
|
|
||
| const missedFilter = filteredMissed === "true" ? true : filteredMissed === "false" ? false : undefined; | ||
|
|
||
| const { data, error, isFetching, isLoading } = useDeadlinesServiceGetDeadlines({ |
There was a problem hiding this comment.
I would ideally like to include the alert reference and the dag run's end_date plus state to give much more context here, but that can be a custom UI endpoint later.
There was a problem hiding this comment.
Should I leave as is for now?
| export const Deadlines = () => { | ||
| const { t: translate } = useTranslation("dashboard"); | ||
| const refetchInterval = useAutoRefresh({ checkPendingRuns: true }); | ||
| const now = dayjs().toISOString(); |
There was a problem hiding this comment.
With the refetchInterval, we will calculate a new now and last24h constantly.
It would be better to move this under the "History" section and use the start and end date provided just like you did with the Overview page. But again, we don't want to do any of this yet.
There was a problem hiding this comment.
Same for this, should I leave as is?
Co-authored-by: Brent Bovenzi <brent.bovenzi@gmail.com>
Sounds good, thank you for your patience. This pr might take some time as I will work on my other pr (have a lot of feedback to address), and it's the final exam month for me :(, so this pr might actually not be updated until around the end of April. |
| const filteredDagId = searchParams.get(SearchParamsKeys.DAG_ID); | ||
| const filteredMissed = searchParams.get(SearchParamsKeys.MISSED); | ||
| const deadlineTimeGte = searchParams.get(SearchParamsKeys.DEADLINE_TIME_GTE); | ||
| const deadlineTimeLte = searchParams.get(SearchParamsKeys.DEADLINE_TIME_LTE); | ||
|
|
||
| const missedFilter = filteredMissed === "true" ? true : filteredMissed === "false" ? false : undefined; | ||
|
|
||
| const { data, error, isFetching, isLoading } = useDeadlinesServiceGetDeadlines({ | ||
| dagId: filteredDagId !== null && filteredDagId !== "" ? filteredDagId : "~", | ||
| dagRunId: "~", | ||
| deadlineTimeGte: deadlineTimeGte ?? undefined, | ||
| deadlineTimeLte: deadlineTimeLte ?? undefined, | ||
| limit: pagination.pageSize, | ||
| missed: missedFilter, | ||
| offset: pagination.pageIndex * pagination.pageSize, | ||
| orderBy, | ||
| }); |
There was a problem hiding this comment.
The Deadlines browse view currently fetches all deadlines across all DAGs/DAG runs (dagId="", dagRunId="") with no default time window. This doesn’t match the PR description (future pending + missed within last 24h) and can also be expensive because the backend query will scan/count potentially large tables when no range filter is applied. Consider setting a default deadline_time_gte (e.g. now minus 24h) when the page loads (and/or updating the PR description if the intent is to show all deadlines).
|
@imrichardwu This PR has been converted to draft because it does not yet meet our Pull Request quality criteria. Issues found:
What to do next:
Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. |
Add a global Deadlines view that displays all pending deadlines where deadline_time is in the future, as well as recently missed deadlines from the last 24 hours across all DAG runs, with clickable links to the corresponding DAG and DAG run for sorting and filtering. The dashboard shows the 5 most recent, and the user can view more in the browser section.
Related to #50501 (comment)
Was generative AI tooling used to co-author this PR?
Claude code for code review and bug fix.
{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.