launchpad-reviewers team mailing list archive

[Colin Watson <mp+410593@xxxxxxxxxxxxxxxxxx>]

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.

Diff comments:

> diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
> index 29b5a2d..7ce2299 100644
> --- a/lib/lp/soyuz/model/queue.py
> +++ b/lib/lp/soyuz/model/queue.py
> @@ -651,7 +651,9 @@ class PackageUpload(SQLBase):
>      @cachedproperty
>      def contains_build(self):
>          """See `IPackageUpload`."""
> -        return bool(self.builds)
> +        return bool(
> +            self.builds or self.package_copy_job.metadata["include_binaries"]
> +        )

`self.package_copy_job` might be None even if `self.builds` is empty (for example, a source upload), so this will need an extra guard for that.

>  
>      @cachedproperty
>      def contains_copy(self):


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/410593
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:expose-whether-copies-in-upload-queues-contain-binaries into launchpad:master.