Skip to content

Filter out Copr builds without SRPM in SQL #2863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m-blaha wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Filter out Copr builds without SRPM in SQL #2863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packit_service/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2238,6 +2238,12 @@ def get_merged_chroots(
"packit_id_per_chroot",
),
)
.filter(
# Exclude builds without build_id - these are builds waiting for SRPM
# or where SRPM build failed, so technically they are not actual Copr
# builds yet.
CoprBuildTargetModel.build_id.isnot(None),
)
.group_by(
CoprBuildTargetModel.build_id,
) # Group by identical element(s)
Expand Down
10 changes: 0 additions & 10 deletions packit_service/service/api/copr_builds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from flask_restx import Namespace, Resource

from packit_service.models import (
BuildStatus,
CoprBuildGroupModel,
CoprBuildTargetModel,
optional_timestamp,
Expand Down Expand Up @@ -35,15 +34,6 @@ def get(self):
first, last = indices()
for build in CoprBuildTargetModel.get_merged_chroots(first, last):
build_info = CoprBuildTargetModel.get_by_build_id(build.build_id, None)
if build_info.status == BuildStatus.waiting_for_srpm:
continue
if (
build_info.status == BuildStatus.failure
and not build_info.build_start_time
and not build_info.build_logs_url
):
# SRPM build failed, it doesn't make sense to list this build
continue
project_info = build_info.get_project()
Comment on lines 35 to 37
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 4, 2025

Choose a reason for hiding this comment

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

medium

This loop currently causes an N+1 query problem. For each build returned by get_merged_chroots, you're making at least two more database queries: get_by_build_id and then get_project (which itself can trigger multiple lazy-loads). This can lead to significant performance degradation, especially with a large number of builds.

To resolve this, I recommend modifying CoprBuildTargetModel.get_merged_chroots to fetch all the necessary information in a single query by using joins and returning all required fields. This would eliminate the need for extra queries inside the loop.

For example, you could extend the query in get_merged_chroots to join with CoprBuildGroupModel, PipelineModel, ProjectEventModel, and GitProjectModel to retrieve fields like project_name, build_submitted_time, web_url, commit_sha, and project details. You would need to use an aggregate function (like min or max) on these additional fields within the group_by clause, since they will be the same for all chroots of a given build.

This would make the API endpoint much more performant.

Copy link
Member Author

@m-blaha m-blaha Nov 4, 2025

Choose a reason for hiding this comment

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

This makes sense, but is out of scope here. If we wanted to optimize the code, it should be tracked as a separate issue.

build_dict = {
"packit_id": build_info.id,
Expand Down
Loading