-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add task instance state and name filter to task instances tab in a dagrun #45215
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a small nit in my view. Else: LGTM
3a2fe8f
to
fe68b6f
Compare
Note: As PR #45312 has been merged, the code formatting rules have changed for new UI. Please rebase and re-run pre-commit checks to ensure that formatting in folder airflow/ui is adjusted. |
fe68b6f
to
e2b6060
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested working as expected and code looks good overall.
A few suggestion before merging.
{ label: "All States", value: "all" }, | ||
{ label: "Scheduled", value: "scheduled" }, | ||
{ label: "Queued", value: "queued" }, | ||
{ label: "Running", value: "running" }, | ||
{ label: "Success", value: "success" }, | ||
{ label: "Restarting", value: "restarting" }, | ||
{ label: "Failed", value: "failed" }, | ||
{ label: "Up For Retry", value: "up_for_retry" }, | ||
{ label: "Up For Reschedule", value: "up_for_reschedule" }, | ||
{ label: "Upstream failed", value: "upstream_failed" }, | ||
{ label: "Skipped", value: "skipped" }, | ||
{ label: "Deferred", value: "deferred" }, | ||
{ label: "Removed", value: "removed" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a task instance can also have no status
=> state is null
, not yet scheduled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-select doesn't seem to allow selecting an option with value as null . Backend seems to accept "none" and transform it as None state in the query. { label: "No status", value: "none" }
helps But selecting "No status" will make UI list it as "None" . Maybe displaying selected options part needs to be special cased to handle "none" .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c67eec9 . Please feel free to suggest if there is a better way to handle this state. Thanks.
@@ -82,12 +85,71 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [ | |||
}, | |||
]; | |||
|
|||
const stateOptions = createListCollection({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding typing here you will be able to remove manual casting as TaskInstanceState
. (And also by handling the None).
createListCollection<{ label: string; value: TaskInstanceState | "all" }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done in c67eec9
{Boolean(hideAdvanced) ? undefined : ( | ||
<Button fontWeight="normal" height="1.75rem" variant="ghost" width={140} {...buttonProps}> | ||
Advanced Search | ||
</Button> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of a SearchBar that actually leverage this feature. (with a working button).
As you mentioned in the description. This can be removed if unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a better practice to make the prop showAdvanced
and force pages to opt in to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some API work done which will be used in advanced search. For now I can keep it as opt out here.
@@ -82,12 +85,71 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [ | |||
}, | |||
]; | |||
|
|||
const stateOptions = createListCollection({ | |||
items: [ | |||
{ label: "All States", value: "all" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be further simplified, by having nothing selected at all 'undefined' value, that is considered all
implicitly so we do not have to manually handle an "all". Because that also mess up the typing to have that extra virtual 'state'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added clearable
by default. Now I have made it so that it's clearable only when a state is selected and it's not shown for "all states" when no state is selected.
@@ -96,13 +158,53 @@ export const TaskInstances = () => { | |||
limit: pagination.pageSize, | |||
offset: pagination.pageIndex * pagination.pageSize, | |||
orderBy, | |||
state: filteredState === null ? undefined : [filteredState], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since state
accepts an array. We should make it a MultiSelect component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done. I am appending to query parameter state
on multiple selection so that it's sent as array to backend for filtering
e2b6060
to
67746c6
Compare
67746c6
to
c67eec9
Compare
A dagrun might have a lot of task instances that are paginated. This PR adds filters to filter by state e.g. failed task instances which is similar to the filter in dagrun page now. I have also added search by task_id or task_display_name so that users can quickly get to the required task instance in a paginated setting. Slightly related issues for grid view to search by name #32239 . This also updates the URLs so that it's shareable.
Notes for reviewer and self :
searchBar
component but doesn't need advanced search as of now as it's not implemented and not useful here where search by task name is only needed. This can be added in future and thehideAdvanced
part can be removed.Screenshot :