launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05036
[Merge] lp:~adeuring/launchpad/bug-739052-9 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-9 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739052-9/+merge/76241
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
--
https://code.launchpad.net/~adeuring/launchpad/bug-739052-9/+merge/76241
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739052-9 into lp:launchpad.
=== 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
