Merge lp:~adeuring/launchpad/bug-739052-9 into lp:launchpad

[Abel Deuring]

[r=lifeless][bug=739052] Use StormRangeFactory for the Distribution:+builds vie

This branch fixes bug 739052: Distribution:+builds timeout

The timeout is the "typical" SQL timeout, which we have more
or less frequently for queries with large resultsets.

The core of the change is that the view for pages like
https://launchpad.net/ubuntu/+builds now uses StormRangeFactory
to retrieve a batch of the result set, instead of the default
ListRangeFactory.

Working on this core fix, I noticed two problems in StormRangeFactory:

1. StormRangeFactory.getSliceByIndex() did not work with plain Storm
ResultSets, only with DecoratedResultSets.

2. StormRangeFactory.rough_length did not work with DISTINCT queries
that are ordered by more than one column.

Both problems are fixed; I also noticed that StormRangeFactory
had accumulated several lines like

    if zope_isinstance(expression, Desc):
        expression = expression.epxr

so I added a helper function plain_expression().

The next change affects the code which generates the ResultSet used
in the view: StormRangeFactory needs result sets where the order
expressions are simple Storm Column instances (or Desc(column).
Things like

    resultset.order_by('Person.id')

do not work.

Also, StormRangeFactory works only with Storm result sets, but
not with legacy SQLBase.select() results.

The result set of "our" view is created by
BinaryPackageBuild.getBuildsByArchIds(), and this method used
resultset.order_by('a string'). Out of paranoia, I also added
BinaryPackageBuild.id as an order_by() argument where the only
sort expression was a timestamp.

I was not sure where to add a test for this change: Since the
change is in model code, the test could be added to the tests
of BinaryPackageBuild itself.

On the other hand, the change is _needed_ for browser code, so
the test can also be added to the unit tests of the view class.

I opted for the latter: The test becomes a bit pointless, should
the view class (or some "intermediate code") decide to no longer
call BinaryPackageBuild.getBuildsByArchIds() but to use instead
something-completely-different.

The test nevertheless makes an assumption about what needs to be
tested: That the sorting depending on the parameter "build status"
but on nothing else.

The last change is a new class DistributionBuildRecordsView, which
is now registered in ZCML as the view for the +builds page of
IDistribtuions: BuildRecordsView itself is used also for
IDistroArchSeries, and we have four classes for other contexts
that are derived from BuildRecordsView.

If I would have changed BuildRecordsView itself to use
StormRangeFactory, I would have had to check that all five other
contexts return result sets with proper order_by() parameters.
And I know there is at least one method that needs to be changed,
so, to keep the size of the change "within limits", I decided
to define a new class property "range_factory", which is
by default ListRangeFactory, while the new class
DistributionBuildRecordsView uses StormrangeFactory.

tests:

./bin/test soyuz -vvt test_build_views

[Robert Collins (lifeless)]

\o/ \o/ \o/ \o/

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-09-14 15:37:11 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-09-20 15:15:27 +0000
@@ -243,6 +243,14 @@
         self.shadow_values.reverse()
 
 
+def plain_expression(expression):
+    """Strip an optional DESC() from an expression."""
+    if isinstance(expression, Desc):
+        return expression.expr
+    else:
+        return expression
+
+
 class StormRangeFactory:
     """A range factory for Storm result sets.
 
@@ -304,8 +312,7 @@
             row = (row, )
         sort_expressions = self.getOrderBy()
         for expression in sort_expressions:
-            if zope_isinstance(expression, Desc):
-                expression = expression.expr
+            expression = plain_expression(expression)
             if not zope_isinstance(expression, PropertyColumn):
                 raise StormRangeFactoryError(
                     'StormRangeFactory only supports sorting by '
@@ -372,8 +379,7 @@
 
         converted_memo = []
         for expression, value in zip(sort_expressions, parsed_memo):
-            if isinstance(expression, Desc):
-                expression = expression.expr
+            expression = plain_expression(expression)
             try:
                 expression.variable_factory(value=value)
             except TypeError, error:
@@ -452,11 +458,6 @@
 
     def equalsExpressionsFromLimits(self, limits):
         """Return a list [expression == memo, ...] for the given limits."""
-        def plain_expression(expression):
-            if isinstance(expression, Desc):
-                return expression.expr
-            else:
-                return expression
 
         result = []
         for expressions, memos in limits:
@@ -570,19 +571,23 @@
     def getSliceByIndex(self, start, end):
         """See `IRangeFactory."""
         sliced = self.resultset[start:end]
-        return ShadowedList(list(sliced), list(sliced.get_plain_result_set()))
+        if zope_isinstance(sliced, DecoratedResultSet):
+            return ShadowedList(
+                list(sliced), list(sliced.get_plain_result_set()))
+        sliced = list(sliced)
+        return ShadowedList(sliced, sliced)
 
     @cachedproperty
     def rough_length(self):
         """See `IRangeFactory."""
         # get_select_expr() requires at least one column as a parameter.
         # getorderBy() already knows about columns that can appear
-        # in the result set, let's take just the first one.
-        column = self.getOrderBy()[0]
-        if zope_isinstance(column, Desc):
-            column = column.expr
+        # in the result set, so let's use them. Moreover, for SELECT
+        # DISTINCT queries, each column used for sorting must appear
+        # in the result.
+        columns = [plain_expression(column) for column in self.getOrderBy()]
         select = removeSecurityProxy(self.plain_resultset).get_select_expr(
-            column)
+            *columns)
         explain = 'EXPLAIN ' + convert_storm_clause_to_string(select)
         store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
         result = store.execute(explain)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2011-09-14 15:37:11 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-09-20 15:15:27 +0000
@@ -747,3 +747,41 @@
         estimated_length = range_factory.rough_length
         self.assertThat(estimated_length, LessThan(10))
         self.assertThat(estimated_length, Not(LessThan(1)))
+
+    def test_rough_length_distinct_query(self):
+        # StormRangeFactory.rough_length with SELECT DISTINCT queries.
+        resultset = self.makeStormResultSet()
+        resultset.config(distinct=True)
+        resultset.order_by(Person.name, Person.id)
+        range_factory = StormRangeFactory(resultset)
+        estimated_length = range_factory.rough_length
+        self.assertThat(estimated_length, LessThan(10))
+        self.assertThat(estimated_length, Not(LessThan(1)))
+
+    def test_getSliceByIndex__storm_result_set(self):
+        # StormRangeFactory.getSliceByIndex() returns a slice of the
+        # resultset, wrapped into a ShadowedList. For plain Storm
+        # result sets, the main values and the shadow values are both
+        # the corresponding elements of the result set.
+        resultset = self.makeStormResultSet()
+        all_results = list(resultset)
+        range_factory = StormRangeFactory(resultset)
+        sliced = range_factory.getSliceByIndex(2, 4)
+        self.assertIsInstance(sliced, ShadowedList)
+        self.assertEqual(all_results[2:4], list(sliced))
+        self.assertEqual(all_results[2:4], sliced.shadow_values)
+
+    def test_getSliceByIndex__decorated_result_set(self):
+        # StormRangeFactory.getSliceByIndex() returns a slice of the
+        # resultset, wrapped into a ShadowedList. For decorated Storm
+        # result sets, the main values are the corresponding values of
+        # the decorated result set and the shadow values are the
+        # corresponding elements of the plain result set.
+        resultset = self.makeDecoratedStormResultSet()
+        all_results = list(resultset)
+        all_undecorated_results = list(resultset.get_plain_result_set())
+        range_factory = StormRangeFactory(resultset)
+        sliced = range_factory.getSliceByIndex(2, 4)
+        self.assertIsInstance(sliced, ShadowedList)
+        self.assertEqual(all_results[2:4], list(sliced))
+        self.assertEqual(all_undecorated_results[2:4], sliced.shadow_values)

=== modified file 'lib/lp/soyuz/browser/build.py'
--- lib/lp/soyuz/browser/build.py	2011-09-13 05:23:16 +0000
+++ lib/lp/soyuz/browser/build.py	2011-09-20 15:15:27 +0000
@@ -14,8 +14,10 @@
     'BuildRescoringView',
     'BuildUrl',
     'BuildView',
+    'DistributionBuildRecordsView',
     ]
 
+from lazr.batchnavigator import ListRangeFactory
 from lazr.delegates import delegates
 from lazr.restful.utils import safe_hasattr
 from zope.component import getUtility
@@ -37,7 +39,10 @@
     stepthrough,
     )
 from canonical.launchpad.webapp.authorization import check_permission
-from canonical.launchpad.webapp.batching import BatchNavigator
+from canonical.launchpad.webapp.batching import (
+    BatchNavigator,
+    StormRangeFactory,
+    )
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
 from lp.app.browser.launchpadform import (
@@ -456,6 +461,8 @@
     # only, but subclasses can set this if desired.
     binary_only = True
 
+    range_factory = ListRangeFactory
+
     @property
     def label(self):
         return 'Builds for %s' % self.context.displayname
@@ -486,7 +493,8 @@
         builds = self.context.getBuildRecords(
             build_state=self.state, name=self.text, arch_tag=self.arch_tag,
             user=self.user, binary_only=binary_only)
-        self.batchnav = BatchNavigator(builds, self.request)
+        self.batchnav = BatchNavigator(
+            builds, self.request, range_factory=self.range_factory(builds))
         # We perform this extra step because we don't what to issue one
         # extra query to retrieve the BuildQueue for each Build (batch item)
         # A more elegant approach should be extending Batching class and
@@ -639,3 +647,12 @@
     @property
     def no_results(self):
         return self.form_submitted and not self.complete_builds
+
+
+class DistributionBuildRecordsView(BuildRecordsView):
+    """See BuildRecordsView."""
+
+    # SQL Queries generated by the default ListRangeFactory time out
+    # for some views, like +builds?build_state=all. StormRangeFactory
+    # is more efficient.
+    range_factory = StormRangeFactory

=== modified file 'lib/lp/soyuz/browser/configure.zcml'
--- lib/lp/soyuz/browser/configure.zcml	2011-09-19 14:29:47 +0000
+++ lib/lp/soyuz/browser/configure.zcml	2011-09-20 15:15:27 +0000
@@ -785,7 +785,7 @@
         </browser:pages>
         <browser:pages
             for="lp.registry.interfaces.distribution.IDistribution"
-            class="lp.soyuz.browser.build.BuildRecordsView"
+            class="lp.soyuz.browser.build.DistributionBuildRecordsView"
             permission="zope.Public">
             <browser:page
                 name="+builds"

=== modified file 'lib/lp/soyuz/browser/tests/test_build_views.py'
--- lib/lp/soyuz/browser/tests/test_build_views.py	2011-07-13 18:54:37 +0000
+++ lib/lp/soyuz/browser/tests/test_build_views.py	2011-09-20 15:15:27 +0000
@@ -8,12 +8,18 @@
     timedelta,
     )
 import pytz
+from testtools.matchers import (
+    MatchesException,
+    Not,
+    Raises,
+    )
 from zope.component import (
     getMultiAdapter,
     getUtility,
     )
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.interfaces import StormRangeFactoryError
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -291,3 +297,44 @@
         expected_url = canonical_url(build)
         browser = self.getUserBrowser(url)
         self.assertEquals(expected_url, browser.url)
+
+    def test_DistributionBuildRecordsView__range_factory(self):
+        # DistributionBuildRecordsView works with StormRangeFactory:
+        # StormRangeFactory requires result sets where the sort order
+        # is specified by Storm Column objects or by Desc(storm_column).
+        # DistributionBuildRecordsView gets its resultset from
+        # IDistribution.getBuildRecords(); the sort order of the
+        # result set depends on the parameter build_state.
+        # The order expressions for all possible values of build_state
+        # are usable by StormRangeFactory.
+        distroseries = self.factory.makeDistroSeries()
+        distribution = distroseries.distribution
+        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
+        for status in BuildStatus.items:
+            build = self.factory.makeBinaryPackageBuild(
+                distroarchseries=das, archive=distroseries.main_archive,
+                status=status)
+            # BPBs in certain states need a bit tweaking to appear in
+            # the result of getBuildRecords().
+            if status == BuildStatus.FULLYBUILT:
+                with person_logged_in(self.admin):
+                    build.date_started = (
+                        datetime.now(pytz.UTC) - timedelta(hours=1))
+                    build.date_finished = datetime.now(pytz.UTC)
+            elif status in (BuildStatus.NEEDSBUILD, BuildStatus.BUILDING):
+                build.queueBuild()
+        for status in ('built', 'failed', 'depwait', 'chrootwait',
+                       'superseded', 'uploadfail', 'all', 'building',
+                       'pending'):
+            view = create_initialized_view(
+                distribution, name="+builds", form={'build_state': status})
+            view.setupBuildList()
+            range_factory = view.batchnav.batch.range_factory
+
+            def test_range_factory():
+                row = range_factory.resultset.get_plain_result_set()[0]
+                range_factory.getOrderValuesFor(row)
+
+            self.assertThat(
+                test_range_factory,
+                Not(Raises(MatchesException(StormRangeFactoryError))))

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2011-08-17 13:43:13 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2011-09-20 15:15:27 +0000
@@ -29,9 +29,9 @@
     EmptyResultSet,
     Store,
     )
+from storm.zope import IResultSet
 from zope.component import getUtility
 from zope.interface import implements
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from canonical.database.sqlbase import (
@@ -1021,16 +1021,21 @@
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
             BuildStatus.UPLOADING]:
-            orderBy = ["-BuildQueue.lastscore", "BinaryPackageBuild.id"]
+            order_by = [Desc(BuildQueue.lastscore), BinaryPackageBuild.id]
+            order_by_table = BuildQueue
             clauseTables.append('BuildQueue')
             clauseTables.append('BuildPackageJob')
             condition_clauses.append(
                 'BuildPackageJob.build = BinaryPackageBuild.id')
             condition_clauses.append('BuildPackageJob.job = BuildQueue.job')
         elif status == BuildStatus.SUPERSEDED or status is None:
-            orderBy = ["-BuildFarmJob.date_created"]
+            order_by = [Desc(BuildFarmJob.date_created),
+                        BinaryPackageBuild.id]
+            order_by_table = BuildFarmJob
         else:
-            orderBy = ["-BuildFarmJob.date_finished"]
+            order_by = [Desc(BuildFarmJob.date_finished),
+                        BinaryPackageBuild.id]
+            order_by_table = BuildFarmJob
 
         # End of duplication (see XXX cprov 2006-09-25 above).
 
@@ -1043,14 +1048,22 @@
             "PackageBuild.archive IN %s" %
             sqlvalues(list(distribution.all_distro_archive_ids)))
 
+        store = Store.of(distribution)
+        clauseTables = [BinaryPackageBuild] + clauseTables
+        result_set = store.using(*clauseTables).find(
+            (BinaryPackageBuild, order_by_table), *condition_clauses)
+        result_set.order_by(*order_by)
+
+        def get_bpp(result_row):
+            return result_row[0]
+
         return self._decorate_with_prejoins(
-            BinaryPackageBuild.select(' AND '.join(condition_clauses),
-            clauseTables=clauseTables, orderBy=orderBy))
+            DecoratedResultSet(result_set, result_decorator=get_bpp))
 
     def _decorate_with_prejoins(self, result_set):
         """Decorate build records with related data prefetch functionality."""
         # Grab the native storm result set.
-        result_set = removeSecurityProxy(result_set)._result_set
+        result_set = IResultSet(result_set)
         decorated_results = DecoratedResultSet(
             result_set, pre_iter_hook=self._prefetchBuildData)
         return decorated_results