hussein-awala commented on code in PR #75216:
URL: https://github.com/apache/airflow/pull/75216#discussion_r4414159791
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -46,11 +46,32 @@
)
from airflow.api_fastapi.core_api.services.public.pools import
BulkPoolService, update_orm_from_pydantic
from airflow.api_fastapi.logging.decorators import action_logging
-from airflow.models.pool import Pool
+from airflow.models.pool import Pool, PoolStats
pools_router = AirflowRouter(tags=["Pool"], prefix="/pools")
+def _build_pool_response(pool: Pool, stats: PoolStats) -> PoolResponse:
+ occupied_slots = stats["running"] + stats["queued"]
+ if pool.include_deferred:
+ occupied_slots += stats["deferred"]
+ return PoolResponse.model_validate(
+ {
+ "pool": pool.pool,
+ "slots": pool.slots,
+ "description": pool.description,
+ "include_deferred": pool.include_deferred,
+ "team_name": pool.team_name,
+ "occupied_slots": occupied_slots,
+ "running_slots": stats["running"],
+ "queued_slots": stats["queued"],
+ "scheduled_slots": stats["scheduled"],
+ "open_slots": stats["open"],
Review Comment:
Could we calculate open_slots from pool.slots and the occupied_slots
computed above?
The page query and slots_stats() read the pool configuration separately.
Under READ COMMITTED, a concurrent capacity change from 10 to 20 (with two
running slots) can produce slots=10, occupied_slots=2, and open_slots=18 in the
same response. Changing include_deferred between the reads causes a similar
inconsistency.
Previously, open_slots() used the same loaded pool configuration. Deriving
it here would preserve that consistency without another query, while retaining
the unlimited-pool handling.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -119,10 +140,22 @@ def get_pools(
session=session,
)
- pools = session.scalars(pools_select)
+ pools = session.scalars(pools_select).all()
+ if not pools:
+ return PoolCollectionResponse(pools=[], total_entries=total_entries)
+
+ pool_stats = Pool.slots_stats(
Review Comment:
Could we preserve the database’s pool-name comparison semantics here?
I confirmed this with real API requests on freshly initialized MySQL. After
deleting an occupied pool named `analytics` and recreating it as `Analytics`,
the running task retains its original pool name. SQL comparisons still match it
under the case-insensitive collation, but `slots_stats()` groups under
`analytics` and misses the Python dictionary key `Analytics`.
The regression test below produces:
| Endpoint | Running | Occupied | Open |
|---|---:|---:|---:|
| Single pool | 2 | 2 | 8 |
| Pool list | 0 | 0 | 10 |
It **fails on this PR** and **passes when only the previous list
implementation is restored**. Could the aggregation join against `Pool` and
group by its canonical name?
Add this test to `TestGetPools` in
`airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py`:
```python
@pytest.mark.backend("mysql")
def test_list_counts_slots_after_pool_is_recreated_with_different_case(
self, test_client, dag_maker, session
):
response = test_client.post("/pools", json={"name": "analytics",
"slots": 10})
assert response.status_code == 201
with dag_maker(dag_id="test_recreated_pool_slot_counts",
session=session):
EmptyOperator(task_id="running_task", pool="analytics", pool_slots=2)
ti = dag_maker.create_dagrun().get_task_instances(session=session)[0]
ti.state = TaskInstanceState.RUNNING
session.commit()
assert test_client.delete("/pools/analytics").status_code == 204
response = test_client.post("/pools", json={"name": "Analytics",
"slots": 10})
assert response.status_code == 201
single_response = test_client.get("/pools/Analytics")
assert single_response.status_code == 200
expected_counts = {"running_slots": 2, "occupied_slots": 2,
"open_slots": 8}
single_pool = single_response.json()
assert {key: single_pool[key] for key in expected_counts} ==
expected_counts
response = test_client.get("/pools", params={"pool_name_pattern":
"Analytics"})
assert response.status_code == 200
pools = response.json()["pools"]
assert len(pools) == 1
assert {key: pools[0][key] for key in expected_counts} == expected_counts
```
Run in an isolated Breeze MySQL database:
```bash
breeze run --backend mysql bash -c '
airflow db reset --yes &&
pytest
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py::TestGetPools::test_list_counts_slots_after_pool_is_recreated_with_different_case
--without-db-init -xvs
'
```
The explicit initialization matters: normal test initialization can use
historical migrations that create binary-collated pool columns. This reproduces
the case-insensitive columns created by Airflow’s normal fresh-install path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
