ColtenOuO opened a new pull request, #71918: URL: https://github.com/apache/airflow/pull/71918
## Summary `BulkVariableService.handle_bulk_update` and `BulkPoolService.handle_bulk_update` (`airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py` and `.../pools.py`) each ran one batched existence-check query (`categorize_keys`/`categorize_pools`) up front, then discarded the result (`_, matched_keys, not_found_keys = ...`) and re-queried every entity individually inside the loop via `update_orm_from_pydantic`. This is the same defect class as two previous PRs of mine: - #66222 "Fix N+1 query pattern in bulk pool delete endpoint" - #67304 "Fix N+1 query in bulk task instance delete endpoint" Both fixed the **delete** side of these bulk endpoints. This PR fixes the same bug on the **update** side, in the same two files — the sibling `handle_bulk_delete` methods (and `connections.py`'s `handle_bulk_update`) already reuse the batched-lookup dict correctly, so this closes the one spot that was missed. ## Changes `update_orm_from_pydantic` in both files now takes the already-fetched ORM object instead of a key/name string, matching the design `connections.py` already uses: | File | Function | Before | After | |---|---|---|---| | `variables.py` | `update_orm_from_pydantic` | fetches `Variable` by key itself | receives the already-fetched `Variable` | | `variables.py` | `BulkVariableService.handle_bulk_update` | discards `categorize_keys()` result, re-queries per key | passes `existing_variables_dict[key]` | | `pools.py` | `update_orm_from_pydantic` | fetches `Pool` by name itself | receives the already-fetched `Pool` | | `pools.py` | `BulkPoolService.handle_bulk_update` | discards `categorize_pools()` result, re-queries per name | passes `existing_pools_dict[name]` | Since `update_orm_from_pydantic` no longer does its own DB lookup, it also can't raise 404 itself anymore — the "fetch + 404" responsibility moved to whichever caller does the fetch: the single-item `patch_variable`/`patch_pool` routes now fetch-then-call (previously they delegated the fetch into the shared function), and the bulk service already resolves not-found entities earlier via `not_found_keys`/`not_found_pool_names`. ## Round-trip reduction Measured directly from the regression tests below (not a synthetic benchmark) — before is the query count with the source fix reverted, after is with it applied: | endpoint | entities updated (N) | before (RTT) | after (RTT) | reduction | |---|---:|---:|---:|---| | bulk update variables | 1 | 2 | 1 | −1 | | bulk update variables | 25 | 26 | 1 | −25 | | bulk update pools | 5 | 9 | 4 | −5 | | bulk update pools | 10 | 14 | 4 | −10 | | bulk update pools | 20 | 24 | 4 | −20 | Both scale exactly linearly before the fix (`variables: N + 1`, `pools: N + 4`) and are flat after it. Extrapolating the same formula: a client bulk-updating 100 variables or pools in one call — e.g. a CI job resetting 100 Airflow Variables, or an admin script raising `slots` on 100 pools — goes from 101 (variables) / 104 (pools) round trips down to 1 / 4, regardless of N. <img width="1278" height="522" alt="image" src="https://github.com/user-attachments/assets/ca312378-33eb-4cca-9e94-a0b181f037f9"; /> ## Blast-radius check `update_orm_from_pydantic`'s parameter type changed (`str` key → ORM object), so I checked every call site across the whole repo, not just these two files: - Repo-wide grep for `update_orm_from_pydantic` finds exactly 3 independent, module-private definitions (`variables.py`, `pools.py`, `connections.py` — untouched, already correct). Each has exactly 2 call sites (the single-item PATCH route and the bulk service), and I updated all 4 affected ones. - No test file calls `update_orm_from_pydantic` directly. - No code outside `airflow-core` (providers, task-sdk, airflow-ctl) imports `services.public.variables` or `services.public.pools` — these are internal implementation modules, not part of any public SDK surface. - The REST API contract (request/response schemas, status codes) is unchanged — this is a Python-level signature change, not an API-level one; the existing route test suites confirm behavior is identical. --- ##### Was generative AI tooling used to co-author this PR? - [X] Yes — Claude Code (Sonnet 5) -- 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]
