launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03671
[Merge] lp:~adeuring/launchpad/bug-739065 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739065 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739065/+merge/61555
This branch is a first step to fix bug 739065: SourcePackage:+index timeouts
The OOPS reports from that bug show that two SQL queries which return only one row each are repeated quite often; this branch lets the query in DistributionSourcePackageRelease.sample_binary_packages prejoin DistroSeriesPackageCache records.
Since it can happen that no DistroSeriesPackageCache exists for a given DistroSeriesBinaryPackage, I also changed its constructor so that passing the optional parameter cache=None means "no cache available", instead of "parameter not specified or no cache available".
A test run of the new query in DistributionSourcePackageRelease.sample_binary_packages indicates that it is slightly faster than the old query; avoiding the retrieval DistroSeriesPackageCache records in separate queries would save more than 3 seconds in OOPS 1904E1683.
DistributionSourcePackageRelease.sample_binary_packages was not tested at all, so I added a few general tests.
tests:
./bin/test soyuz -vvt test_distributionsourcepackagerelease
./bin/test soyuz -vvt test_distroseriesbinarypackage
no lint
--
https://code.launchpad.net/~adeuring/launchpad/bug-739065/+merge/61555
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739065 into lp:launchpad.
=== modified file 'lib/lp/soyuz/model/distributionsourcepackagerelease.py'
--- lib/lp/soyuz/model/distributionsourcepackagerelease.py 2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/model/distributionsourcepackagerelease.py 2011-05-19 12:41:50 +0000
@@ -12,11 +12,20 @@
]
from lazr.delegates import delegates
-from storm.expr import Desc
+from storm.expr import (
+ And,
+ Desc,
+ Join,
+ LeftJoin,
+ SQL,
+ )
from zope.component import getUtility
from zope.interface import implements
from canonical.database.sqlbase import sqlvalues
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
from canonical.launchpad.webapp.interfaces import (
DEFAULT_FLAVOR,
IStoreSelector,
@@ -34,6 +43,7 @@
from lp.soyuz.model.binarypackagename import BinaryPackageName
from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
from lp.soyuz.model.distroseriesbinarypackage import DistroSeriesBinaryPackage
+from lp.soyuz.model.distroseriespackagecache import DistroSeriesPackageCache
from lp.soyuz.model.publishing import (
BinaryPackagePublishingHistory,
SourcePackagePublishingHistory,
@@ -152,33 +162,59 @@
@property
def sample_binary_packages(self):
"""See IDistributionSourcePackageRelease."""
- all_published = BinaryPackagePublishingHistory.select("""
- BinaryPackagePublishingHistory.distroarchseries =
- DistroArchSeries.id AND
- DistroArchSeries.distroseries = DistroSeries.id AND
- DistroSeries.distribution = %s AND
- BinaryPackagePublishingHistory.archive IN %s AND
- BinaryPackagePublishingHistory.binarypackagerelease =
- BinaryPackageRelease.id AND
- BinaryPackageRelease.binarypackagename = BinaryPackageName.id AND
- BinaryPackageRelease.build = BinaryPackageBuild.id AND
- BinaryPackageBuild.source_package_release = %s
- """ % sqlvalues(self.distribution,
- self.distribution.all_distro_archive_ids,
- self.sourcepackagerelease),
- distinct=True,
- orderBy=['BinaryPackageName.name'],
- clauseTables=['DistroArchSeries', 'DistroSeries',
- 'BinaryPackageRelease', 'BinaryPackageName',
- 'BinaryPackageBuild'],
- prejoinClauseTables=['BinaryPackageRelease', 'BinaryPackageName'])
+ #avoid circular imports.
+ from lp.registry.model.distroseries import DistroSeries
+ from lp.soyuz.model.distroarchseries import DistroArchSeries
+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ archive_ids = list(self.distribution.all_distro_archive_ids)
+ result_row = (
+ SQL('DISTINCT ON(BinaryPackageName.name) 0 AS ignore'),
+ BinaryPackagePublishingHistory, DistroSeriesPackageCache,
+ BinaryPackageRelease, BinaryPackageName)
+ tables = (
+ BinaryPackagePublishingHistory,
+ Join(
+ DistroArchSeries,
+ DistroArchSeries.id ==
+ BinaryPackagePublishingHistory.distroarchseriesID),
+ Join(
+ DistroSeries,
+ DistroArchSeries.distroseriesID == DistroSeries.id),
+ Join(
+ BinaryPackageRelease,
+ BinaryPackageRelease.id ==
+ BinaryPackagePublishingHistory.binarypackagereleaseID),
+ Join(
+ BinaryPackageName,
+ BinaryPackageName.id ==
+ BinaryPackageRelease.binarypackagenameID),
+ Join(
+ BinaryPackageBuild,
+ BinaryPackageBuild.id == BinaryPackageRelease.buildID),
+ LeftJoin(
+ DistroSeriesPackageCache,
+ And(
+ DistroSeriesPackageCache.distroseries == DistroSeries.id,
+ DistroSeriesPackageCache.archiveID.is_in(archive_ids),
+ DistroSeriesPackageCache.binarypackagename ==
+ BinaryPackageName.id)))
+
+ all_published = store.using(*tables).find(
+ result_row,
+ DistroSeries.distribution == self.distribution,
+ BinaryPackagePublishingHistory.archiveID.is_in(archive_ids),
+ BinaryPackageBuild.source_package_release ==
+ self.sourcepackagerelease)
+ all_published = all_published.order_by(
+ BinaryPackageName.name)
+ all_published = DecoratedResultSet(
+ all_published, lambda row: row[1:3])
+
samples = []
- names = set()
- for publishing in all_published:
- if publishing.binarypackagerelease.binarypackagename not in names:
- names.add(publishing.binarypackagerelease.binarypackagename)
- samples.append(
- DistroSeriesBinaryPackage(
- publishing.distroarchseries.distroseries,
- publishing.binarypackagerelease.binarypackagename))
+ for publishing, package_cache in all_published:
+ samples.append(
+ DistroSeriesBinaryPackage(
+ publishing.distroarchseries.distroseries,
+ publishing.binarypackagerelease.binarypackagename,
+ package_cache))
return samples
=== modified file 'lib/lp/soyuz/model/distroseriesbinarypackage.py'
--- lib/lp/soyuz/model/distroseriesbinarypackage.py 2010-10-24 12:46:23 +0000
+++ lib/lp/soyuz/model/distroseriesbinarypackage.py 2011-05-19 12:41:50 +0000
@@ -39,10 +39,12 @@
implements(IDistroSeriesBinaryPackage)
- def __init__(self, distroseries, binarypackagename, cache=None):
+ default = object()
+
+ def __init__(self, distroseries, binarypackagename, cache=default):
self.distroseries = distroseries
self.binarypackagename = binarypackagename
- if cache is not None:
+ if cache is not self.default:
get_property_cache(self).cache = cache
@property
@@ -69,9 +71,9 @@
self.distroseries.distribution.all_distro_archive_ids)
result = store.find(
DistroSeriesPackageCache,
- DistroSeriesPackageCache.distroseries==self.distroseries,
+ DistroSeriesPackageCache.distroseries == self.distroseries,
DistroSeriesPackageCache.archiveID.is_in(archive_ids),
- (DistroSeriesPackageCache.binarypackagename==
+ (DistroSeriesPackageCache.binarypackagename ==
self.binarypackagename))
return result.any()
=== added file 'lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py'
--- lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_distributionsourcepackagerelease.py 2011-05-19 12:41:50 +0000
@@ -0,0 +1,152 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests of DistributionSourcePackageRelease."""
+
+from testtools.matchers import LessThan
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.interfaces.distroarchseries import IDistroArchSeriesSet
+from lp.soyuz.model.distributionsourcepackagerelease import (
+ DistributionSourcePackageRelease,
+ )
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import (
+ StormStatementRecorder,
+ TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestDistributionSourcePackageRelease(TestCaseWithFactory):
+ """Tests for DistributionSourcePackageRelease."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionSourcePackageRelease, self).setUp()
+ self.sourcepackagerelease = self.factory.makeSourcePackageRelease()
+ self.distroarchseries = self.factory.makeDistroArchSeries(
+ distroseries=self.sourcepackagerelease.sourcepackage.distroseries)
+ distribution = self.distroarchseries.distroseries.distribution
+ self.dsp_release = DistributionSourcePackageRelease(
+ distribution, self.sourcepackagerelease)
+
+ def makeBinaryPackageRelease(self, name=None):
+ if name is None:
+ name = self.factory.makeBinaryPackageName()
+ bp_build = self.factory.makeBinaryPackageBuild(
+ source_package_release=self.sourcepackagerelease,
+ distroarchseries=self.distroarchseries)
+ bp_release = self.factory.makeBinaryPackageRelease(
+ build=bp_build, binarypackagename=name)
+ sourcepackagename = self.sourcepackagerelease.sourcepackagename
+ self.factory.makeSourcePackagePublishingHistory(
+ sourcepackagename,
+ sourcepackagerelease=self.sourcepackagerelease,
+ distroseries=self.sourcepackagerelease.sourcepackage.distroseries,
+ status=PackagePublishingStatus.PUBLISHED)
+ self.factory.makeBinaryPackagePublishingHistory(
+ binarypackagerelease=bp_release,
+ distroarchseries=self.distroarchseries)
+
+ def test_sample_binary_packages__no_releases(self):
+ # If no binary releases exist,
+ # DistributionSourcePackageRelease.sample_binary_packages is empty.
+ self.assertEqual([], self.dsp_release.sample_binary_packages)
+
+ def test_sample_binary_packages__one_release(self):
+ # If a binary release exists,
+ # DistributionSourcePackageRelease.sample_binary_packages
+ # returns it.
+ self.makeBinaryPackageRelease(
+ self.factory.makeBinaryPackageName(name='binary-package'))
+ self.assertEqual(
+ ['binary-package'],
+ [release.name
+ for release in self.dsp_release.sample_binary_packages])
+
+ def test_sample_binary_packages__two_releases_one_binary_package(self):
+ # If two binary releases with the same name exist,
+ # DistributionSourcePackageRelease.sample_binary_packages
+ # returns only one.
+ name = self.factory.makeBinaryPackageName(name='binary-package')
+ self.makeBinaryPackageRelease(name)
+ self.makeBinaryPackageRelease(name)
+ self.assertEqual(
+ ['binary-package'],
+ [release.name
+ for release in self.dsp_release.sample_binary_packages])
+
+ def test_sample_binary_packages__two_release_two_binary_packages(self):
+ # If a two binary releases with different names exist,
+ # DistributionSourcePackageRelease.sample_binary_packages
+ # returns both.
+ self.makeBinaryPackageRelease(
+ self.factory.makeBinaryPackageName(name='binary-package'))
+ self.makeBinaryPackageRelease(
+ self.factory.makeBinaryPackageName(name='binary-package-2'))
+ self.assertEqual(
+ ['binary-package', 'binary-package-2'],
+ [release.name
+ for release in self.dsp_release.sample_binary_packages])
+
+ def updateDistroSeriesPackageCache(self):
+ # Create DistroSeriesPackageCache records for new binary
+ # packages.
+ #
+ # SoyuzTestPublisher.updateDistroSeriesPackageCache() creates
+ # a DistroSeriesPackageCache record for the new binary package.
+ # The method closes the current DB connection, making references
+ # to DB objects in other DB objects unusable. Starting with
+ # the distroarchseries, we can create new, valid, instances of
+ # objects required later in the test again.
+ # of the objects we need later.
+ sourcepackagename = self.sourcepackagerelease.sourcepackagename
+ publisher = SoyuzTestPublisher()
+ publisher.updateDistroSeriesPackageCache(
+ self.distroarchseries.distroseries)
+ self.distroarchseries = getUtility(IDistroArchSeriesSet).get(
+ self.distroarchseries.id)
+ distribution = self.distroarchseries.distroseries.distribution
+ releases = distribution.getCurrentSourceReleases([sourcepackagename])
+ [(distribution_sourcepackage, dsp_release)] = releases.items()
+ self.dsp_release = dsp_release
+ self.sourcepackagerelease = dsp_release.sourcepackagerelease
+
+ def test_sample_binary_packages__constant_number_sql_queries(self):
+ # Retrieving
+ # DistributionSourcePackageRelease.sample_binary_packages and
+ # accessing the property "summary" of its items requires a
+ # constant number of SQL queries, regardless of the number
+ # of existing binary package releases.
+ self.makeBinaryPackageRelease()
+ self.updateDistroSeriesPackageCache()
+ with StormStatementRecorder() as recorder:
+ for ds_package in self.dsp_release.sample_binary_packages:
+ ds_package.summary
+ self.assertThat(recorder, HasQueryCount(LessThan(5)))
+ self.assertEqual(1, len(self.dsp_release.sample_binary_packages))
+
+ for iteration in range(5):
+ self.makeBinaryPackageRelease()
+ self.updateDistroSeriesPackageCache()
+ with StormStatementRecorder() as recorder:
+ for ds_package in self.dsp_release.sample_binary_packages:
+ ds_package.summary
+ self.assertThat(recorder, HasQueryCount(LessThan(5)))
+ self.assertEqual(6, len(self.dsp_release.sample_binary_packages))
+
+ # Even if the cache is not updated for binary packages,
+ # DistributionSourcePackageRelease objects do not try to
+ # retrieve DistroSeriesPackageCache records if they know
+ # that such records do not exist.
+ for iteration in range(5):
+ self.makeBinaryPackageRelease()
+ with StormStatementRecorder() as recorder:
+ for ds_package in self.dsp_release.sample_binary_packages:
+ ds_package.summary
+ self.assertThat(recorder, HasQueryCount(LessThan(5)))
+ self.assertEqual(11, len(self.dsp_release.sample_binary_packages))
=== modified file 'lib/lp/soyuz/tests/test_distroseriesbinarypackage.py'
--- lib/lp/soyuz/tests/test_distroseriesbinarypackage.py 2010-12-20 03:21:03 +0000
+++ lib/lp/soyuz/tests/test_distroseriesbinarypackage.py 2011-05-19 12:41:50 +0000
@@ -16,7 +16,10 @@
from lp.services.log.logger import BufferLogger
from lp.soyuz.model.distroseriesbinarypackage import DistroSeriesBinaryPackage
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ StormStatementRecorder,
+ TestCaseWithFactory,
+ )
class TestDistroSeriesBinaryPackage(TestCaseWithFactory):
@@ -61,3 +64,22 @@
self.failUnlessEqual(
'Foo is the best', self.distroseries_binary_package.summary)
+
+ def test_none_cache_passed_at_init_counts_as_cached(self):
+ # If the value None is passed as the constructor parameter
+ # "cache", it is considered as a valid value.
+ # Accesing the property DistroSeriesBinaryPackage.cache
+ # later does not lead to the execution of an SQL query to
+ # retrieve a DistroSeriesPackageCache record.
+ binary_package = DistroSeriesBinaryPackage(
+ self.distroseries, self.binary_package_name, cache=None)
+ with StormStatementRecorder() as recorder:
+ binary_package.cache
+ self.assertEqual(0, len(recorder.statements))
+
+ # If the parameter "cache" was not passed, accessing
+ # DistroSeriesBinaryPackage.cache for the first time requires
+ # at least one SQL query.
+ with StormStatementRecorder() as recorder:
+ self.distroseries_binary_package.cache
+ self.assertTrue(len(recorder.statements) > 0)
