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

AIP-81 Add Insert Multiple Pools API #44121

Conversation

jason810496
Copy link
Contributor

closes: #43896
related: #43657

Fix: Add max_length=256 for PoolPostBody.pool

The model defined in https://github.com/apache/airflow/blob/main/airflow/models/pool.py#L54 enforces a string length constraint on Pool.pool. To maintain consistency, this constraint should also be validated at the router level.

Refactor: Handle Duplicate Cases in Insert Pool

The Insert Pool (single insert) functionality should account for cases where a duplicate pool name is provided, as Pool.pool is defined with a unique constraint.

Feat: Add Insert Multiple Pools API

A new API endpoint for inserting multiple pools has been introduced as /pools/bulk. Alternative names such as /pools/batch or /pools/multiple were considered. Feedback on which name best describes the endpoint is welcome.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 18, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice thanks

airflow/api_fastapi/core_api/routes/public/pools.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/pools.py Outdated Show resolved Hide resolved
@jason810496 jason810496 force-pushed the feature/AIP-81/add-bulk-insert-pools-api branch from 9fa560e to 42e8a51 Compare November 18, 2024 12:53
@bugraoz93 bugraoz93 added the area:API Airflow's REST/HTTP API label Nov 19, 2024
@bugraoz93
Copy link
Collaborator

Looks good! Thanks for the changes @jason810496! I believe the CI failure is likely a transient error caused by a DNS issue that couldn’t resolve AWS S3 at that moment.

@Lee-W
Copy link
Member

Lee-W commented Nov 20, 2024

Looks good! Thanks for the changes @jason810496! I believe the CI failure is likely a transient error caused by a DNS issue that couldn’t resolve AWS S3 at that moment.

just reran. seems to work fine 👀

@jason810496 jason810496 force-pushed the feature/AIP-81/add-bulk-insert-pools-api branch from 42e8a51 to 1ec682f Compare November 20, 2024 12:20
@jason810496
Copy link
Contributor Author

Just resolved the issue with inserting duplicate pools by handling the database exception rather than adding extra logic to the datamodels or performing additional database queries.
By the way, I’m happy to work further on refactoring to handle duplicate insertion cases across all endpoints 🙌

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looking good, one minor nit, we can merge then.

tests/api_fastapi/core_api/routes/public/test_pools.py Outdated Show resolved Hide resolved
pierrejeambrun
pierrejeambrun previously approved these changes Nov 20, 2024
@pierrejeambrun
Copy link
Member

By the way, I’m happy to work further on refactoring to handle duplicate insertion cases across all endpoints 🙌

That would be great. You can create an issue to track that if you're not going to work on it directly just so we do not forget about it. If you plan on tackling that right away, no need to create an issue opening a PR is perfectly fine.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

maybe this is not the best approach

airflow/api_fastapi/core_api/routes/public/pools.py Outdated Show resolved Hide resolved
@jason810496 jason810496 force-pushed the feature/AIP-81/add-bulk-insert-pools-api branch from bde6004 to 510e373 Compare November 21, 2024 07:47
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Really nice. I like the exception handler class approach (private classes, only expose array of handlers), and the factory pattern for registering the handlers, good idea.

A couple of nitpicks, ready to merge after that.

airflow/api_fastapi/common/exceptions.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/pools.py Outdated Show resolved Hide resolved
@jason810496 jason810496 force-pushed the feature/AIP-81/add-bulk-insert-pools-api branch from 510e373 to c9b67c1 Compare November 22, 2024 07:59
@jason810496
Copy link
Contributor Author

Just fixed the nit. I will refactor all endpoints to adopt the global error handler for managing unique constraint violation errors in further PR.

@pierrejeambrun
Copy link
Member

Just fixed the nit. I will refactor all endpoints to adopt the global error handler for managing unique constraint violation errors in further PR.

Nice. 🎉

What is great about that is the exception_handler is global to the application. All endpoints should benefit from it without any additional specifications. The only thing I can thing of is to correctly document the 409 that can be possibly returned. (That's the benefit of this approach)

@pierrejeambrun pierrejeambrun merged commit f33166a into apache:main Nov 22, 2024
45 checks passed
@jedcunningham jedcunningham changed the title AIP-81 Add Insert Multiple Pools API AIP-84 Add Insert Multiple Pools API Dec 18, 2024
@jedcunningham jedcunningham changed the title AIP-84 Add Insert Multiple Pools API AIP-81 Add Insert Multiple Pools API Dec 18, 2024
@jedcunningham jedcunningham added the AIP-81 Enhanced Security in CLI via Integration of API label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-81 Enhanced Security in CLI via Integration of API area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIP-81 Implement POST/Insert Multiple Pools in FastAPI
6 participants