Repository navigation
Batch Team lookups when syncing Dag bundles to the database - #73154
Open
Eason09053360 wants to merge 7 commits into
Open
Eason09053360 wants to merge 7 commits into
Eason09053360 wants to merge 7 commits into
Conversation
Resolving bundle ownership cost one query per configured bundle, and reading the stored bundles' team relationship lazy-loaded one more each. Both scale with the number of configured bundles and run on every Dag processor start, while the surrounding code in the same function already reads its rows in a single query.
Eason09053360
requested review from
ephraimbuddy and
jedcunningham
as code owners
September 15, 2026 02:15
vincbeck
reviewed
Sep 15, 2026
Reading the stored ownership through the helper the model already exposes keeps this function consistent with the other callers that resolve bundle teams, rather than growing a second way to do the same thing. Clearing the team of a bundle that has none is a no-op, but the assignment still had to load the relationship to find out. Deactivated bundles are never removed from dag_bundle, so that load was paid again on every subsequent sync and grew with the number of bundles ever configured.
This was referenced Sep 25, 2026
This was referenced Sep 25, 2026
Contributor
Author
|
cc @vincbeck friendly pin. |
vincbeck
previously approved these changes
Sep 28, 2026
Multi-team is off in most deployments, yet every bundle sync still read the team ownership of every stored bundle. With the mode off, the bundle config cannot name a team and the Dag processor already ignores bundle ownership, which is why its own lookup through the same helper returns early. While multi-team is off, team links left over from an earlier multi-team setup are no longer cleared; the first sync after the mode is turned back on reconciles them.
vincbeck
reviewed
Sep 28, 2026
With multi-team off the bundle config cannot name a team, so building the set of configured team names on every sync was work that could never find anything. Keeping it under the same multi-team check as the stored ownership read leaves no team handling to run in that mode.
team.name compares case-insensitively on MySQL, and the old per-team equality lookup relied on it: a bundle config naming Team-A resolved to the stored team-a. The batched lookup still matches that row, but its result is keyed by the stored name, so the config value missed it and the sync rejected the config with "Team 'Team-A' does not exist". Migration 0131 lower-cases stored team names, so configs written before it hit this on upgrade.
vincbeck
reviewed
Sep 29, 2026
The stored ownership is what the sync compares against to take a team off a bundle. Skipping the read when multi-team is off left ownership from an earlier multi-team setup in place, and requires_access_dag still passes that team to the auth manager, so SimpleAuthManager denied those Dags to every non-admin user. One query per sync is cheap next to that.
On MySQL team.name compares case-insensitively, so the batched IN lookup already returns the stored row for a config value that differs only in case. Finding that row in the result removes the extra query per such bundle on every sync. Stored names are lower case and unique, as teams create, teams sync and migration 0131 enforce, so casefolded keys cannot collide. On case-sensitive backends a differently-cased name now also resolves when another bundle configures the stored spelling.
jason810496
reviewed
Nov 7, 2026
jason810496
left a comment
jason810496
left a comment
Member
There was a problem hiding this comment.
Thanks for the improvement, LGTM overall, waiting or @vincbeck for the final call.
| team: Team | None = None | ||
| if config.team_name: | ||
| team = session.scalars(select(Team).where(Team.name == config.team_name)).one_or_none() | ||
| team = teams_by_name.get(config.team_name.casefold()) |
Member
There was a problem hiding this comment.
Not sure would it make more sense to normalize at the _bundle_config level?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
DagBundlesManager.sync_bundles_to_dbruns on every Dag processor start and queriedteamonce per configuredteam_nameand once per stored bundle, through the lazyDagBundleModel.teamsrelationship. Deactivated bundles are never deleted, so these queries only grow.What
DagBundleModel.get_team_names, and configured teams with oneTeam.name.in_(...). Sinceteamsis no longer loaded, the deactivation loop clears it only on bundles that have a team.teamqueries at one on a fresh DB and two once bundles are stored, including after deactivation.team.nameis a plainString), soTeam-Ain the bundle config matchedteam-a. That now fails, asairflow teams syncalready does. Happy to add a fallback.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5) following the guidelines