Skip to content

AIP-84: Migrating GET queued asset events for DAG to fastAPI - #43934

Merged
pierrejeambrun merged 18 commits into
apache:mainfrom
astronomer:AIP84-get-all-queued-events-for-dag
Nov 16, 2024
Merged

AIP-84: Migrating GET queued asset events for DAG to fastAPI#43934
pierrejeambrun merged 18 commits into
apache:mainfrom
astronomer:AIP84-get-all-queued-events-for-dag

Conversation

@amoghrajesh

@amoghrajeshamoghrajesh commented Nov 12, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

related: #42370

Migrating the GET queued asset events for a DAG endpoint to fast API.

Setup:

  1. Created producer and consumer dags:
    image

  2. Paused one producer while running the other to generate queued events
    image

Responses:

  1. Legacy
    image

  2. FastAPI
    image

With time filtering:

  1. Legacy
    image

(Beyond creation time)
image

  1. FastAPI
    image

(Beyond creation time)
image

Swagger spec: (maintained under dag section)
image

image

^ 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-cyborgboring-cyborgBot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Nov 12, 2024
@amoghrajeshamoghrajesh self-assigned this Nov 13, 2024
@amoghrajesh
amoghrajesh marked this pull request as ready for review November 13, 2024 08:29
@amoghrajeshamoghrajesh added the legacy api Whether legacy API changes should be allowed in PR label Nov 13, 2024
This was referenced Nov 13, 2024
Closed
Closed
@amoghrajesh
amoghrajeshforce-pushed the AIP84-get-all-queued-events-for-dag branch 2 times, most recently from 3af2873 to 2cd5dc1CompareNovember 14, 2024 07:54
@pierrejeambrunpierrejeambrun mentioned this pull request Nov 14, 2024
Merged
@amoghrajesh

amoghrajesh commented Nov 15, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@pierrejeambrun FYI, I have pushed the changes for all the "resolved" comments. The rest are open for input from you.

EDIT: I kept the endpoint at public/dags.py cos if I move to assets, the path will become: /public/assets/{dag_id}/assets/queuedEvent due to the underlying router registration. I moved the datamodels and tests to assets.py however

vatsrahul1001
vatsrahul1001 reviewed Nov 15, 2024
View reviewed changes
Comment threadtests/api_fastapi/core_api/routes/public/test_assets.py Outdated
This was referenced Nov 15, 2024
Closed
Closed
@amoghrajeshamoghrajesh mentioned this pull request Nov 15, 2024
Closed
pierrejeambrun
pierrejeambrun reviewed Nov 15, 2024
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small suggestions / questions almost ready

Comment threadairflow/api_fastapi/common/utils.py Outdated
Comment threadairflow/api_fastapi/core_api/routes/public/dags.py Outdated
pierrejeambrun
pierrejeambrun approved these changes Nov 15, 2024
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As many of other PRs depends on this one, to try to make them move forward I added a few commits, I think it's ready

amoghrajesh
amoghrajesh commented Nov 15, 2024
View reviewed changes
Comment threadairflow/api_fastapi/common/parameters.py
Comment threadairflow/api_fastapi/common/parameters.py
Comment threadairflow/api_fastapi/core_api/routes/public/assets.py
@pierrejeambrun
pierrejeambrun merged commit 3917730 into apache:mainNov 16, 2024
@pierrejeambrun
pierrejeambrun deleted the AIP84-get-all-queued-events-for-dag branch November 16, 2024 00:02
@tirkarthitirkarthi mentioned this pull request Nov 16, 2024
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Nov 16, 2024
@potiuk
Revert "AIP-84: Migrating GET queued asset events for DAG to fastAPI (a…
6c2852a
…pache#43934)"
This reverts commit 3917730.
@potiukpotiuk mentioned this pull request Nov 16, 2024
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Nov 16, 2024
@potiuk
Revert "AIP-84: Migrating GET queued asset events for DAG to fastAPI (a…
e34cb42
…pache#43934)"
This reverts commit 3917730.
potiuk added a commit to potiuk/airflow that referenced this pull request Nov 16, 2024
@potiuk
Revert "AIP-84: Migrating GET queued asset events for DAG to fastAPI (a…
a42cf9c
…pache#43934)"
This reverts commit 3917730.
gopidesupavan pushed a commit that referenced this pull request Nov 16, 2024
@potiuk
Revert "AIP-84: Migrating GET queued asset events for DAG to fastAPI (#…
b5f5646
…43934)" (#44088)
This reverts commit 3917730.
kandharvishnu pushed a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
@amoghrajesh@pierrejeambrunkandharvishnuu
AIP-84: Migrating GET queued asset events for DAG to fastAPI (apache#…
559d6e4
…43934)
* AIP-84: Migrating GET queued asset events for DAG to fastAPI
* adding test cases
* adding setup and teardown
* review comments part 1
* introducing _generate_queued_event_where_clause
* changing to bad request
* adding paginated_select
* moving _generate_queued_event_where_clause to assets.py
* moving datamodels to assets
* moving tests to assets
* reuse fixtures from TestGetAssets and inherit classes to simplify
* Move route to assets module
* Small adjustments
---------
Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
kandharvishnu pushed a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
@potiukkandharvishnuu
Revert "AIP-84: Migrating GET queued asset events for DAG to fastAPI (a…
ab23301
…pache#43934)" (apache#44088)
This reverts commit 3917730.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vatsrahul1001vatsrahul1001vatsrahul1001 left review comments
@pierrejeambrunpierrejeambrunpierrejeambrun approved these changes
@bbovenzibbovenziAwaiting requested review from bbovenzi

Assignees

@amoghrajeshamoghrajesh

Labels

area:APIAirflow's REST/HTTP APIarea:UIRelated to UI/UX. For Frontend Developers.legacy apiWhether legacy API changes should be allowed in PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@amoghrajesh@pierrejeambrun@vatsrahul1001