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/queue.py (+18/-1)
lib/lp/soyuz/tests/test_packageupload.py (+16/-4)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+410593@code.launchpad.net

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 https://paste.ubuntu.com/p/sRbZSXs3zN/ - 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 https://git.launchpad.net/ubuntu-archive-tools/tree/queue. 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: https://paste.ubuntu.com/p/3gy3mm8pW2/

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/queue.py b/lib/lp/soyuz/model/queue.py
2index 29b5a2d..8dac27a 100644
3--- a/lib/lp/soyuz/model/queue.py
4+++ b/lib/lp/soyuz/model/queue.py
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
18
19 @cachedproperty
20 def contains_copy(self):
21@@ -1621,8 +1628,10 @@ class PackageUploadSet:
22
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
31
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)
36+
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/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py
45index 9bd9a66..12d8c1f 100644
46--- a/lib/lp/soyuz/tests/test_packageupload.py
47+++ b/lib/lp/soyuz/tests/test_packageupload.py
48@@ -666,6 +666,13 @@ class TestPackageUploadWithPackageCopyJob(TestCaseWithFactory):
49 pu, pcj = self.makeUploadWithPackageCopyJob()
50 self.assertIs(None, pu.sourcepackagerelease)
51
52+ def test_upload_contains_build_works_with_copy_jobs(self):
53+ pu = self.factory.makeCopyJobPackageUpload()
54+ self.assertFalse(pu.contains_build)
55+
56+ pu = self.factory.makeCopyJobPackageUpload(include_binaries=True)
57+ self.assertTrue(pu.contains_build)
58+
59
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)
64
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)
69+
70+ def make_uploads():
71+ self.makeBinaryPackageUpload(person, component=self.universe)
72+ self.makeCopyJobPackageUpload(person)
73+
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+ )
81+
82 self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
83
84 def test_api_package_upload_log(self):

Subscribers

People subscribed via source and target branches

to status/vote changes: