PR#2299: hub: query_buildroots have to return ASAP - koji

koji

#2299 hub: query_buildroots have to return ASAP

Merged 3 years ago by mikem. Opened 3 years ago by tkopecek.

tkopecek/koji issue2298 into master

hub: query_buildroots have to return ASAP

Tomas Kopecek • 3 years ago

db41061

hub/kojihub.py

file modified

		`@@ -5381,6 +5381,8 @@`
		`values=locals())`
		`result = set(query.execute())`
		`candidate_buildroot_ids = result`
		`+ if not candidate_buildroot_ids:`
		`+ return []`

		`if archiveID is not None:`
		`joins.insert(0, 'buildroot_archives ON buildroot.id = buildroot_archives.buildroot_id')`
		`@@ -5393,6 +5395,8 @@`
		`candidate_buildroot_ids &= result`
		`else:`
		`candidate_buildroot_ids = result`
		`+ if not candidate_buildroot_ids:`
		`+ return []`

		`if taskID is not None:`
		`clauses.append('standard_buildroot.task_id = %(taskID)i')`
		`@@ -5404,6 +5408,8 @@`
		`candidate_buildroot_ids &= result`
		`else:`
		`candidate_buildroot_ids = result`
		`+ if not candidate_buildroot_ids:`
		`+ return []`

		`if candidate_buildroot_ids:`
		`candidate_buildroot_ids = list(candidate_buildroot_ids)`

tkopecek commented 3 years ago

If candidate_buildroot_ids are pruned to zero in some point,
query_buildroots have to return. Otherwise big working queryset will be
created eating resources and returning empty list anyway.

Fixes: https://pagure.io/koji/issue/2298

mikem commented 3 years ago

This changes the behavior in the case where both an rpmID and an archiveID is given. Before these conditions were effectively ORed, because the second stanza would add to candidate_buildroot_ids. With this change, if rpmID is given and that first stanza yields no buildroots, then we return None, even if the archiveID param might yield some.

~~Granted, I'm not 100% what we should be doing when both are given, or if we even really want to support that.~~

Edited 3 years ago by mikem

mikem commented 3 years ago

Nevermind, I think I'm reading this wrong

mikem commented 3 years ago

Looking at this, I realize that the original optimization from #2074 leaves the original joins and clauses in place for the rpmID and archiveID cases, even though they are essentially obsoleted.

This means two things,

In the "not used in buildroot" case this PR is addressing, the behavior should be no worse than before #2074
In these cases, we are still performing these joins, which seems like a potential performance hit.

Granted, we do need to perform the join to return the is_update field in the rpmID case (though I wonder if we actually need that field). Otoh, the join in the archiveID case seems entirely redundant.

Edited 3 years ago by mikem

mikem commented 3 years ago

I guess we do not have to address the above here, though. This fix looks fine. We just might want to follow up.

:thumbsup:

mikem commented 3 years ago

Commit 9fa07f6 fixes this pull-request