Ines Almeida has proposed merging
~ines-almeida/launchpad:fetch-service-update-build-metadata-url into
launchpad:master.
Commit message:
Add DB load bulk load for `build_metadata_url` attributes when getting multiple
builds
This reduces the number of DB queries made in the case for where a snap has
multiple builds.
Also: revert commit that commented out a test that verified the snap.builds
queries number
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464839
The test that was previously commented out now runs successfully.
Also added a new test, and re-ran the old `build_metadata_url` related tests
succesfully.
--
Your team Launchpad code reviewers is requested to review the proposed merge of
~ines-almeida/launchpad:fetch-service-update-build-metadata-url into
launchpad:master.
diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index ea7bf99..842b142 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -6,6 +6,7 @@ __all__ = [
"SnapFile",
]
+from collections import defaultdict
from datetime import timedelta, timezone
from operator import attrgetter
@@ -51,7 +52,7 @@ from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.person import Person
from lp.services.config import config
-from lp.services.database.bulk import load_related
+from lp.services.database.bulk import load_referencing, load_related
from lp.services.database.constants import DEFAULT
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.enumcol import DBEnum
@@ -427,12 +428,15 @@ class SnapBuild(PackageBuildMixin, StormBase):
return [self.lfaUrl(lfa) for _, lfa, _ in self.getFiles()]
@property
- def build_metadata_url(self):
- metadata_filename = BUILD_METADATA_FILENAME_FORMAT.format(
+ def metadata_filename(self):
+ return BUILD_METADATA_FILENAME_FORMAT.format(
build_id=self.build_cookie
)
+
+ @cachedproperty
+ def build_metadata_url(self):
try:
- return self.lfaUrl(self.getFileByName(metadata_filename))
+ return self.lfaUrl(self.getFileByName(self.metadata_filename))
except NotFoundError:
return None
@@ -663,6 +667,24 @@ class SnapBuildSet(SpecificBuildFarmJobSourceMixin):
)
load_related(Job, sbjs, ["job_id"])
+ # Prefetch all snaps metadata files
+ snap_files = load_referencing(SnapFile, builds, ["snapbuild_id"])
+ lfas = load_related(LibraryFileAlias, snap_files, ["libraryfile_id"])
+
+ metadata_files = defaultdict(list)
+ for snap_file in snap_files:
+ if (
+ snap_file.libraryfile.filename
+ == snap_file.snapbuild.metadata_filename
+ ):
+ metadata_files[snap_file.snapbuild_id] = snap_file.libraryfile
+
+ for build in builds:
+ cache = get_property_cache(build)
+ cache.build_metadata_url = build.lfaUrl(
+ metadata_files.get(build.id)
+ )
+
def getByBuildFarmJobs(self, build_farm_jobs):
"""See `ISpecificBuildFarmJobSource`."""
if len(build_farm_jobs) == 0:
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index eace8dd..7aa8c29 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -5798,70 +5798,121 @@ class TestSnapWebservice(TestCaseWithFactory):
self.webservice.get(url)
self.assertThat(recorder, HasQueryCount(Equals(15)))
- # XXX ines-almeida 2024-04-22: after adding the new API attribute
- # `build_metadata_url` to the snap builds, we actually perform an extra
- # query per build. We need to make a decision on whether we are OK with
- # this (and in such case, this test should be reworked or eventually
- # removed)
- #
- # def test_builds_query_count(self):
- # # The query count of Snap.builds is constant in the number of
- # # builds, even if they have store upload jobs.
- # self.pushConfig(
- # "snappy",
- # store_url="http://sca.example/";,
- # store_upload_url="http://updown.example/";,
- # )
- # with admin_logged_in():
- # snappyseries = self.factory.makeSnappySeries()
- # distroseries = self.factory.makeDistroSeries(
- # distribution=getUtility(IDistributionSet)["ubuntu"],
- # registrant=self.person,
- # )
- # processor = self.factory.makeProcessor(supports_virtualized=True)
- # distroarchseries = self.makeBuildableDistroArchSeries(
- # distroseries=distroseries, processor=processor, owner=self.person
- # )
- # with person_logged_in(self.person):
- # snap = self.factory.makeSnap(
- # registrant=self.person,
- # owner=self.person,
- # distroseries=distroseries,
- # processors=[processor],
- # )
- # snap.store_series = snappyseries
- # snap.store_name = self.factory.getUniqueUnicode()
- # snap.store_upload = True
- # snap.store_secrets = {"root": Macaroon().serialize()}
- # builds_url = "%s/builds" % api_url(snap)
- # logout()
- #
- # def make_build():
- # with person_logged_in(self.person):
- # builder = self.factory.makeBuilder()
- # build = snap.requestBuild(
- # self.person,
- # distroseries.main_archive,
- # distroarchseries,
- # PackagePublishingPocket.PROPOSED,
- # )
- # with dbuser(config.builddmaster.dbuser):
- # build.updateStatus(
- # BuildStatus.BUILDING, date_started=snap.date_created
- # )
- # build.updateStatus(
- # BuildStatus.FULLYBUILT,
- # builder=builder,
- # date_finished=(
- # snap.date_created + timedelta(minutes=10)
- # ),
- # )
- # return build
- #
- # def get_builds():
- # response = self.webservice.get(builds_url)
- # self.assertEqual(200, response.status)
- # return response
- #
- # recorder1, recorder2 = record_two_runs(get_builds, make_build, 2)
- # self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+ def test_builds_query_count(self):
+ # The query count of Snap.builds is constant regardless of the number
+ # of builds, even if they have store upload jobs.
+ self.pushConfig(
+ "snappy",
+ store_url="http://sca.example/";,
+ store_upload_url="http://updown.example/";,
+ )
+ with admin_logged_in():
+ snappyseries = self.factory.makeSnappySeries()
+ distroseries = self.factory.makeDistroSeries(
+ distribution=getUtility(IDistributionSet)["ubuntu"],
+ registrant=self.person,
+ )
+ processor = self.factory.makeProcessor(supports_virtualized=True)
+ distroarchseries = self.makeBuildableDistroArchSeries(
+ distroseries=distroseries, processor=processor, owner=self.person
+ )
+ with person_logged_in(self.person):
+ snap = self.factory.makeSnap(
+ registrant=self.person,
+ owner=self.person,
+ distroseries=distroseries,
+ processors=[processor],
+ )
+ snap.store_series = snappyseries
+ snap.store_name = self.factory.getUniqueUnicode()
+ snap.store_upload = True
+ snap.store_secrets = {"root": Macaroon().serialize()}
+ builds_url = "%s/builds" % api_url(snap)
+ logout()
+
+ def make_build():
+ with person_logged_in(self.person):
+ builder = self.factory.makeBuilder()
+ build = snap.requestBuild(
+ self.person,
+ distroseries.main_archive,
+ distroarchseries,
+ PackagePublishingPocket.PROPOSED,
+ )
+ with dbuser(config.builddmaster.dbuser):
+ build.updateStatus(
+ BuildStatus.BUILDING, date_started=snap.date_created
+ )
+ build.updateStatus(
+ BuildStatus.FULLYBUILT,
+ builder=builder,
+ date_finished=(
+ snap.date_created + timedelta(minutes=10)
+ ),
+ )
+ return build
+
+ def get_builds():
+ response = self.webservice.get(builds_url)
+ self.assertEqual(200, response.status)
+ return response
+
+ recorder1, recorder2 = record_two_runs(get_builds, make_build, 2)
+ self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+ def test_builds_metadata_url(self):
+ # The DB load for the `build_metadata_url` attribute of builds returns
+ # the expected value
+ with admin_logged_in():
+ snappyseries = self.factory.makeSnappySeries()
+ distroseries = self.factory.makeDistroSeries(
+ distribution=getUtility(IDistributionSet)["ubuntu"],
+ registrant=self.person,
+ )
+ processor = self.factory.makeProcessor(supports_virtualized=True)
+ distroarchseries = self.makeBuildableDistroArchSeries(
+ distroseries=distroseries, processor=processor, owner=self.person
+ )
+ with person_logged_in(self.person):
+ snap = self.factory.makeSnap(
+ registrant=self.person,
+ owner=self.person,
+ distroseries=distroseries,
+ processors=[processor],
+ )
+ snap.store_series = snappyseries
+ snap.store_name = self.factory.getUniqueUnicode()
+ snap.store_upload = True
+ snap.store_secrets = {"root": Macaroon().serialize()}
+ builds_url = "%s/builds" % api_url(snap)
+ logout()
+
+ with person_logged_in(self.person):
+ build = snap.requestBuild(
+ self.person,
+ distroseries.main_archive,
+ distroarchseries,
+ PackagePublishingPocket.PROPOSED,
+ )
+ snap1_lfa = self.factory.makeLibraryFileAlias(
+ filename="test.snap",
+ content=b"dummy snap content",
+ db_only=True,
+ )
+ metadata_filename = f"{build.build_cookie}_metadata.json"
+ snap2_lfa = self.factory.makeLibraryFileAlias(
+ filename=metadata_filename,
+ content=b"dummy snap content",
+ db_only=True,
+ )
+ self.factory.makeSnapFile(snapbuild=build, libraryfile=snap2_lfa)
+ self.factory.makeSnapFile(snapbuild=build, libraryfile=snap1_lfa)
+
+ response = self.webservice.get(builds_url)
+ self.assertEqual(200, response.status)
+ snap_builds = (response.jsonBody()["entries"],)
+ self.assertEqual(1, len(snap_builds))
+ self.assertIn(
+ metadata_filename,
+ snap_builds[0][0]["build_metadata_url"],
+ )
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
