launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

I concur that there shouldn't usually be a concerning number of BuildQueues, particularly since we never suspend those belonging to SnapBuilds.

BuildQueue.destroySelf() is easy to bulkify if needed in the future.

Diff comments:

> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2016-05-16 14:33:34 +0000
> +++ lib/lp/snappy/model/snap.py	2016-05-17 11:00:11 +0000
> @@ -424,6 +426,19 @@
>          """See `ISnap`."""
>          store = IStore(Snap)
>          store.find(SnapArch, SnapArch.snap == self).remove()
> +        # Remove build jobs.  There won't be many queued builds, so we can
> +        # afford to do this the safe but slow way via BuildQueue.destroySelf
> +        # rather than in bulk.
> +        buildqueue_records = store.find(
> +            BuildQueue,
> +            BuildQueue._build_farm_job_id == SnapBuild.build_farm_job_id,
> +            SnapBuild.snap == self)
> +        for buildqueue_record in buildqueue_records:
> +            buildqueue_record.destroySelf()
> +        build_farm_job_ids = list(store.find(
> +            BuildFarmJob.id,
> +            BuildFarmJob.id == SnapBuild.build_farm_job_id,
> +            SnapBuild.snap == self))

BuildFarmJob is a useless join here, and this set could conceivably be quite large.

>          # XXX cjwatson 2016-02-27 bug=322972: Requires manual SQL due to
>          # lack of support for DELETE FROM ... USING ... in Storm.
>          store.execute("""


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-properly-delete-builds/+merge/294897
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.