[Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-update-build-metadata-url into launchpad:master
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
[Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-update-build-metadata-url into launchpad:master
The proposal to merge ~ines-almeida/launchpad:fetch-service-update-build-metadata-url into launchpad:master has been updated. Commit message changed to: Update `build_metadat_url` logic and tests and queries associated with it This includes: - Update the query used to get the metadata file to be lighter - Remove test that no longer applies For more details, see: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464751 -- 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. ___ 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
[Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-update-build-metadata-url into launchpad:master
Ines Almeida has proposed merging
~ines-almeida/launchpad:fetch-service-update-build-metadata-url into
launchpad:master.
Commit message:
Update `build_metadat_url` logic and tests and queries associated with it
This includes:
- Update the query used to get the metadata file to be lighter
- Remove test that no longer applies
- Pre-load librarian file data when fetching all builds
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464751
All `build_metadata_url` tests re-ran.
I'm not 100% set if there is more to be done regarding keeping the query count
stable regardless of builds, unless we store the link to the librarian file
directly in the SnapBuild DB table. I'd love opinions on this.
--
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/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index 1d08e03..9bacf76 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -11,6 +11,7 @@ token if and only if they are allowed general internet access.
"""
__all__ = [
+"BUILD_METADATA_FILENAME_FORMAT",
"BuilderProxyMixin",
]
diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index c7418e6..0fc5d16 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -51,7 +51,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
@@ -431,10 +431,10 @@ class SnapBuild(PackageBuildMixin, StormBase):
metadata_filename = BUILD_METADATA_FILENAME_FORMAT.format(
build_id=self.build_cookie
)
-for url in self.getFileUrls():
-if url.endswith(metadata_filename):
-return url
-return None
+try:
+return self.lfaUrl(self.getFileByName(metadata_filename))
+except NotFoundError:
+return None
@cachedproperty
def eta(self):
@@ -663,6 +663,10 @@ class SnapBuildSet(SpecificBuildFarmJobSourceMixin):
)
load_related(Job, sbjs, ["job_id"])
+# Prefetch snap files
+snap_files = load_referencing(SnapFile, builds, ["snapbuild_id"])
+load_related(LibraryFileAlias, snap_files, ["libraryfile_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 9f671b8..39ba808 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -5797,65 +5797,3 @@ class TestSnapWebservice(TestCaseWithFactory):
with StormStatementRecorder() as recorder:
self.webservice.get(url)
self.assertThat(recorder, HasQueryCount(Equals(15)))
-
-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(
-
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-update-build-metadata-url into launchpad:master
Diff comments: > diff --git a/lib/lp/snappy/model/snapbuild.py > b/lib/lp/snappy/model/snapbuild.py > index c7418e6..0fc5d16 100644 > --- a/lib/lp/snappy/model/snapbuild.py > +++ b/lib/lp/snappy/model/snapbuild.py > @@ -663,6 +663,10 @@ class SnapBuildSet(SpecificBuildFarmJobSourceMixin): > ) > load_related(Job, sbjs, ["job_id"]) > > +# Prefetch snap files I'm not 100% sure about this. Regarding query counts, it didn't seem to have an effect. I added it as a commit that I can revert, but I'd like comments on this if there is any. > +snap_files = load_referencing(SnapFile, builds, ["snapbuild_id"]) > +load_related(LibraryFileAlias, snap_files, ["libraryfile_id"]) > + > def getByBuildFarmJobs(self, build_farm_jobs): > """See `ISpecificBuildFarmJobSource`.""" > if len(build_farm_jobs) == 0: -- https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464751 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. ___ 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
