Skip to content

Fix REST API pools: serialize unlimited open_slots as -1 instead of inf#65466

Merged
pierrejeambrun merged 3 commits into
apache:v2-11-testfrom
shan-zeeshan786:pools-api-inf-open-slots
Apr 29, 2026
Merged

Fix REST API pools: serialize unlimited open_slots as -1 instead of inf#65466
pierrejeambrun merged 3 commits into
apache:v2-11-testfrom
shan-zeeshan786:pools-api-inf-open-slots

Conversation

@shan-zeeshan786
Copy link
Copy Markdown

@shan-zeeshan786 shan-zeeshan786 commented Apr 18, 2026

For pools with unlimited capacity (slots == -1), Pool.open_slots() returns float("inf"). The Connexion v1 REST API schema defines open_slots as an integer, so the serialized response failed OpenAPI validation and clients saw HTTP 500 with a message that inf is not an integer.

This change updates PoolSchema.get_open_slots so unlimited pools expose open_slots as -1, aligned with how unlimited capacity is already represented via slots: -1. A regression test ensures serialization never emits inf for such pools.

Files changed

  • airflow/api_connexion/schemas/pool_schema.py — map inf to -1 when dumping open_slots; return type is int.
  • tests/api_connexion/schemas/test_pool_schemas.py — test for a pool with slots=-1 expecting open_slots=-1.

Related issue
close: #65377

Copy link
Copy Markdown
Contributor

@henry3260 henry3260 left a comment

Choose a reason for hiding this comment

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

Overall looks great! Just a small nit

Comment thread tests/api_connexion/schemas/test_pool_schemas.py Outdated
Copy link
Copy Markdown
Contributor

@henry3260 henry3260 left a comment

Choose a reason for hiding this comment

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

Looks good, just waiting for a maintainer's review.

Copy link
Copy Markdown
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.

LGTM, thanks

@pierrejeambrun pierrejeambrun added the _eol_backport-to-v2-11-test Mark PR with this label to backport to v2-11-test branch label Apr 20, 2026
@pierrejeambrun pierrejeambrun added this to the Airflow 2.11.3 milestone Apr 20, 2026
@pierrejeambrun
Copy link
Copy Markdown
Member

CI need fixing.

@shan-zeeshan786
Copy link
Copy Markdown
Author

@pierrejeambrun — CI didn't auto-trigger on the latest commit. Could you rerun?

Copy link
Copy Markdown
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.

Apparently CI still needs fixing. You can take a look at our contribution documentation to see how to run those check locally.

@shan-zeeshan786
Copy link
Copy Markdown
Author

shan-zeeshan786 commented Apr 22, 2026

Apparently CI still needs fixing. You can take a look at our contribution documentation to see how to run those check locally.

Hi @pierrejeambrun — I dug into the three failing jobs on commit 41395ab and they all appear to be pre-existing on v2-11-stable, not caused by this PR (which only modifies airflow/api_connexion/schemas/pool_schema.py +15/-3 and tests/api_connexion/schemas/test_pool_schemas.py +22/-3). Concretely:

  1. Non-DB providers (fab) fails at orchestration with Unknown test type for GroupOfTests.PROVIDERS: fab (before any test runs). The fab provider test type isn't registered in this branch's test-type registry.
  2. Build documentation --docs-only fails with 297 Sphinx errors — the first ones are undefined label: 'howto/connection:gcp' in production-deployment.rst:252,257 and toctree contains reference to nonexisting document '_api/tests/system/core/index' in index.rst:180. None of these .rst files are touched by this PR.
  3. Build documentation --spellcheck-only fails with 697 of the same Sphinx errors plus 1 unrelated spelling error.

The static-checks job (which originally was the only one failing because of my code) is now green after 41395ab. Could you confirm whether the three remaining failures need a separate branch fix, or whether they can be ignored for this PR? Happy to follow up if any of them turns out to be related to my change.

@potiuk potiuk marked this pull request as draft April 22, 2026 19:53
@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 22, 2026

@shan-zeeshan786 Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.

  • Failing CI: seven checks are failing across several provider test matrices: MySQL / DB-providers, Sqlite / DB-providers, Postgres / DB-providers, Lowest direct dependency providers tests, Non-DB tests / providers — plus Build documentation and spellcheck-only. The broad provider impact suggests the serialization change touches a field that providers depend on. Reproduce locally with breeze testing providers-tests --run-in-parallel and investigate the failing DB-providers tests first to find the root cause; the docs failures are likely separate and easy to chase by running breeze build-docs.

See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush.


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@pierrejeambrun pierrejeambrun marked this pull request as ready for review April 28, 2026 12:54
Copy link
Copy Markdown
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.

I believe that only the static failure was caused by this PR.

I just restarted the CI lets see 🤞, should be good.

@pierrejeambrun pierrejeambrun changed the base branch from v2-11-stable to v2-11-test April 29, 2026 12:15
@pierrejeambrun
Copy link
Copy Markdown
Member

pierrejeambrun commented Apr 29, 2026

Updated the target branch from v2-11-stable to v2-11-test. @shan-zeeshan786 PR should always target *-test branch, stable is for release when stuff have passed some checks. That could be the source of the CI failure too.

Let's see what happens now.

@pierrejeambrun pierrejeambrun force-pushed the pools-api-inf-open-slots branch from 41395ab to ed03dec Compare April 29, 2026 12:16
@pierrejeambrun pierrejeambrun merged commit cf2fe46 into apache:v2-11-test Apr 29, 2026
45 checks passed
@boring-cyborg
Copy link
Copy Markdown

boring-cyborg Bot commented Apr 29, 2026

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@shan-zeeshan786
Copy link
Copy Markdown
Author

Thanks @pierrejeambrun , i will take care of your points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

_eol_backport-to-v2-11-test Mark PR with this label to backport to v2-11-test branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants