Merge ~jugmac00/launchpad:expose-whether-copies-in-upload-queues-contain-binaries into launchpad:master

Proposed by Jürgen Gmach
Status: Merged
Approved by: Jürgen Gmach
Approved revision: f350ab9c07a5c0cafe9fe72089180d770742922c
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~jugmac00/launchpad:expose-whether-copies-in-upload-queues-contain-binaries
Merge into: launchpad:master
Diff against target: 84 lines (+34/-5)
2 files modified
lib/lp/soyuz/model/ (+18/-1)
lib/lp/soyuz/tests/ (+16/-4)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email:

Commit message

expose whether copies in upload queues contain binaries

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

This is a partial review because I ran out of time before my long weekend, but I'll give you what I've got.

I think this may have the problem that, if a webservice request needs to fetch a collection of uploads and render their exported properties (of which `contains_build` is one), then this will incur an extra `PackageCopyJob` query for each such upload. I ran out of time trying to construct a test that would prove this, although I at least verified that it seems to be the case by stepping through some relevant bits of code. My partial attempt was - perhaps you can make something of that.

A fix will probably involve adding some more preloading to `lp.soyuz.model.queue.prefill_packageupload_caches`; I suggest comparing with `lp.soyuz.browser.queue.QueueItemsView.loadPackageCopyJobs` (called from the immediately-following `decoratedQueueBatch`), which does something similar for the web UI case; you may not need quite as much preloading as that, and if you can manage to construct a failing test along the lines of what I wrote then you can be guided by that.

However, `PackageUpload.copy_source_archive` already has a similar problem (apparently my fault in 2013), so this branch isn't making things any worse. As a result, I think the above would be good to fix but shouldn't be a blocker.

However however: I think we need to consider whether it's OK to put this in `contains_build`, or whether we need an additional property of some kind. The main client tool that consumes this is If we need to change it too then that's OK, but we should work out whether this change will be backwards-compatible. I'm happy to help further with this once I'm back.

Revision history for this message
Colin Watson (cjwatson) wrote :

After some more debugging (I'd forgotten that the test suite normally sets the batch size to 5, so the batches being retrieved by `getPackageUploads` were the same size on each run and so didn't show any query scaling problems), here's a test diff that exhibits the problem:

62261be... by Jürgen Gmach

preload related PackageCopyJob data

... in order to avoid extra queries.

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

Thank you for your excellent feedback (and preparation).

MP is ready for review.

Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
Jürgen Gmach (jugmac00) :
Revision history for this message
Colin Watson (cjwatson) :
f350ab9... by Jürgen Gmach

Use the dedicated `include_binaries` property

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/soyuz/model/ b/lib/lp/soyuz/model/
2index 29b5a2d..8dac27a 100644
3--- a/lib/lp/soyuz/model/
4+++ b/lib/lp/soyuz/model/
5@@ -651,7 +651,14 @@ class PackageUpload(SQLBase):
6 @cachedproperty
7 def contains_build(self):
8 """See `IPackageUpload`."""
9- return bool(self.builds)
10+ if self.builds:
11+ return True
12+ else:
13+ # handle case when PackageUpload is a copy
14+ copy_job = self.concrete_package_copy_job
15+ if copy_job is not None:
16+ return copy_job.include_binaries
17+ return False
19 @cachedproperty
20 def contains_copy(self):
21@@ -1621,8 +1628,10 @@ class PackageUploadSet:
23 def prefill_packageupload_caches(uploads, puses, pubs, pucs, logs):
24 # Circular imports.
25+ from lp.registry.model.distribution import Distribution
26 from lp.soyuz.model.archive import Archive
27 from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
28+ from lp.soyuz.model.packagecopyjob import PackageCopyJob
29 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
30 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
32@@ -1684,3 +1693,11 @@ def prefill_packageupload_caches(uploads, puses, pubs, pucs, logs):
33 spr_cache.published_archives.append(publication.archive)
34 for diff in diffs:
35 get_property_cache(diff.to_source).package_diffs.append(diff)
37+ package_copy_jobs = load_related(
38+ PackageCopyJob, uploads, ['package_copy_job_id']
39+ )
40+ archives = load_related(
41+ Archive, package_copy_jobs, ['source_archive_id']
42+ )
43+ load_related(Distribution, archives, ['distributionID'])
44diff --git a/lib/lp/soyuz/tests/ b/lib/lp/soyuz/tests/
45index 9bd9a66..12d8c1f 100644
46--- a/lib/lp/soyuz/tests/
47+++ b/lib/lp/soyuz/tests/
48@@ -666,6 +666,13 @@ class TestPackageUploadWithPackageCopyJob(TestCaseWithFactory):
49 pu, pcj = self.makeUploadWithPackageCopyJob()
50 self.assertIs(None, pu.sourcepackagerelease)
52+ def test_upload_contains_build_works_with_copy_jobs(self):
53+ pu = self.factory.makeCopyJobPackageUpload()
54+ self.assertFalse(pu.contains_build)
56+ pu = self.factory.makeCopyJobPackageUpload(include_binaries=True)
57+ self.assertTrue(pu.contains_build)
60 class TestPackageUploadCustom(TestCase):
61 """Unit tests for `PackageUploadCustom`."""
62@@ -1474,13 +1481,18 @@ class TestPackageUploadWebservice(TestCaseWithFactory):
63 self.assertEndsWith(ws_upload.copy_source_archive_link, archive_url)
65 def test_getPackageUploads_query_count(self):
66+ self.pushConfig("launchpad", default_batch_size=20)
67 person = self.makeQueueAdmin([self.universe])
68 ws_distroseries = self.load(self.distroseries, person)
70+ def make_uploads():
71+ self.makeBinaryPackageUpload(person, component=self.universe)
72+ self.makeCopyJobPackageUpload(person)
74 recorder1, recorder2 = record_two_runs(
75- ws_distroseries.getPackageUploads,
76- lambda: self.makeBinaryPackageUpload(
77- person, component=self.universe),
78- 5)
79+ ws_distroseries.getPackageUploads, make_uploads, 5
80+ )
82 self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
84 def test_api_package_upload_log(self):


People subscribed via source and target branches

to status/vote changes: