Skip to content
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 "queuedEvent" endpoint to get/delete DatasetDagRunQueue #37176

Merged
merged 15 commits into from Feb 20, 2024

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Feb 5, 2024

Why

In #36618, a user asked for a way to delete DatasetDagRunQueue and devised a workaround that directly manipulated the airflow database. Thus, in this PR, I'm trying to create API endpoints to provide a way to do so without the user needing to directly manipulate the DB

What

Add the following API endpoints to get/delete DatasetDagRunQueue with an argument before, which allows users to filter only the DatasetDagRunQueue created before this time.

  • DELETE /dags/{dag_id}/datasets/queuedEvent/{uri}
  • DELETE /dags/{dag_id}/datasets/queuedEvent
  • DELETE /datasets/queuedEvent/{uri}
  • GET /dags/{dag_id}/datasets/queuedEvent/{uri}
  • GET /dags/{dag_id}/datasets/queuedEvent
  • GET /datasets/queuedEvent/{uri}

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Feb 5, 2024
@Lee-W Lee-W marked this pull request as ready for review February 5, 2024 13:56
@Lee-W
Copy link
Member Author

Lee-W commented Feb 5, 2024

I came up a few questions during implementation

  1. should we use uri instead of dataset_id in this endpoint? but we'll have to use Dataset to find its id and then query DDRQ.
  2. What kind of permission would be ideal for this endpoint? or should we add a new permission for this endpoint?
  3. Should we add thecutoff_time in this API? The original thought was to add more flexibility so that user can delete only the DDRQ created before this cutoff_time

@Lee-W Lee-W marked this pull request as draft February 6, 2024 11:30
@Lee-W
Copy link
Member Author

Lee-W commented Feb 6, 2024

I came up a few questions during implementation

1. should we use `uri` instead of `dataset_id` in this endpoint? but we'll have to use Dataset to find its id and then query DDRQ.

2. What kind of permission would be ideal for this endpoint? or should we add a new permission for this endpoint?

3. Should we add the`cutoff_time` in this API? The original thought was to add more flexibility so that user can delete only the DDRQ created before this `cutoff_time`

Discussed with @jedcunningham earlier today and make the following changes according to Jed's suggestion (Thanks Jed!)

  1. Change the original endpoint to DELETE /dags/{dag_id}/datasets/{uri}/pendingEvents
  2. Add the following endpoints
    1. DELETE /dags/{dag_id}/datasets/pendingEvents
    2. DELETE /datasets/{uri}/pendingEvents
    3. GET /dags/{dag_id}/datasets/{uri}/pendingEvents
    4. GET /dags/{dag_id}/datasets/pendingEvents
    5. GET /datasets/{uri}/pendingEvents
  3. Rename cutoff_time as before
  4. Change DELETE to use query args provide before instead
  5. Permission-wise, we can use requires_access_dag and requires_access_dataset

For point 5, @vincbeck , we're wondering whether you have any thought on it. Thanks!

@Lee-W Lee-W marked this pull request as ready for review February 6, 2024 11:36
@vincbeck
Copy link
Contributor

vincbeck commented Feb 6, 2024

5. requires_access_dataset

I would use requires_access_dataset

@Lee-W
Copy link
Member Author

Lee-W commented Feb 7, 2024

  1. requires_access_dataset

I would use requires_access_dataset

Thanks for your suggestions! Just updated it.

Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be good for @bbovenzi to have a look. i suspect there wants to be a UI component here for management of the queue if it gets to undesirable state. @cmarteepants is that part of the plan?

airflow/api_connexion/openapi/v1.yaml Show resolved Hide resolved
airflow/api_connexion/openapi/v1.yaml Show resolved Hide resolved
@Lee-W Lee-W changed the title add endpoint to delete DatasetDagRunQueue add "queuedEvent" endpoint to get/delete DatasetDagRunQueue Feb 19, 2024
@Lee-W
Copy link
Member Author

Lee-W commented Feb 19, 2024

For the content in .significant, I use 1.1.0 as the minimum FAB version as I think it should be a MINOR update. Another thing that comes to my mind is whether we need to move the FAB change into another PR as this PR currently contains changes on both core and provider.

@Lee-W Lee-W force-pushed the add-delete-DDRQ-endpoint branch 2 times, most recently from 7aa1615 to 60861a8 Compare February 20, 2024 02:15
@Lee-W
Copy link
Member Author

Lee-W commented Feb 20, 2024

Hi @vincbeck @potiuk @eladkal , I would like to confirm whether we should move the FAB change (adding a new delete dataset permission) to a separate PR. Or are we good to just keep it in this PR? Will it affect the release of 2.9 or the FAB provider? Many thanks!

@uranusjr
Copy link
Member

I think it probably does not matter either way. The provider release only includes provider code, so only the provider changes of this PR will be included anyway.

@Lee-W
Copy link
Member Author

Lee-W commented Feb 20, 2024

I think it probably does not matter either way. The provider release only includes provider code, so only the provider changes of this PR will be included anyway.

Got it. If that's the case, should/could we merge this PR? or do we want to wait for more response on this? Thanks!

@vincbeck vincbeck merged commit 16d2671 into apache:main Feb 20, 2024
59 checks passed
sunank200 pushed a commit to astronomer/airflow that referenced this pull request Feb 21, 2024
@jedcunningham jedcunningham deleted the add-delete-DDRQ-endpoint branch February 26, 2024 20:44
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants