-
Notifications
You must be signed in to change notification settings - Fork 1.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
filter by multiple statuses in BulkActionsFilter #23772
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @jamiedemaria and the rest of your teammates on Graphite |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 401a3f5. |
246f841
to
667ee37
Compare
95df830
to
1bf6d09
Compare
@prha @sryza The main thing with this PR is if this is even worth adding. We can filter runs by multiple statuses, so we want to be able to filter backfills by multiple statuses. But we filter runs by DagsterRun statuses, so that's the status we'll eventually filter backfills by. I was thinking it could be useful to also allow filtering backfills by multiple BulkActionStatuses to enable us to do non-performant filtering before we store the dagster run status in the DB, but it might not really be worth it Related - I'm wondering if we should have a more verbose name for the |
@@ -841,14 +841,16 @@ def get_backfills( | |||
) -> Sequence[PartitionBackfill]: | |||
check.opt_inst_param(status, "status", BulkActionStatus) | |||
query = db_select([BulkActionsTable.c.body, BulkActionsTable.c.timestamp]) |
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 we should go more aggressively.
We should deprecate status
and error if they provide both status
AND filters
667ee37
to
24dafe8
Compare
1bf6d09
to
ab35503
Compare
24dafe8
to
c7ee34c
Compare
ab35503
to
d14e094
Compare
c7ee34c
to
868bc51
Compare
d14e094
to
a491770
Compare
a491770
to
401a3f5
Compare
Summary & Motivation
You can select multiple statuses when you filter for runs in the UI, so we should support filtering backfills for multiple statuses. Alternatively we can wait until the dagster run status for a backfill is stored in the DB and then just allow filtering for multiple statuses of dagster run statuses
companion internal pr https://github.com/dagster-io/internal/pull/11113
How I Tested These Changes
unit tests