launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07146
[Merge] lp:~wgrant/launchpad/bugtaskflat-search-2 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bugtaskflat-search-2 into lp:launchpad with lp:~wgrant/launchpad/bugtaskflat-search-1 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bugtaskflat-search-2/+merge/102421
Due to the perilous performance characteristics of bug search, we want to be able to switch BugTaskFlat usage on or off quickly with a feature flag. This entails *temporarily* making lp.bugs.model.bugtasksearch a hybrid creature, able to operate on either.
This branch introduces the first parts of the actual hybridisation: a feature flag is introduced which causes bugtasksearch to base itself on BugTaskFlat instead. Filtering and sorting is still done entirely on Bug and BugTask, so a few ugly implicit joins are required at this stage, and it will be far too slow for production.
There is a bit of ugliness in search_bugs to make the underlying queries simpler. When using BugTaskFlat to retrieve BugTasks, we actually query out BugTaskFlat.bugtask_id and use DecoratedResultSet to map them to BugTasks efficiently when iterated. This implies some further ugliness for compatibility with the eager-loading pre_iter_hook that the caller can provide, since it expects BugTasks rather than IDs.
test_bugtask_search now runs both flat and non-flat variants of the tests, which means that this already very slow test module will be even slower. But it's only for the short term.
--
https://code.launchpad.net/~wgrant/launchpad/bugtaskflat-search-2/+merge/102421
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bugtaskflat-search-2 into lp:launchpad.
=== added file 'lib/lp/bugs/model/bugtaskflat.py'
--- lib/lp/bugs/model/bugtaskflat.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/model/bugtaskflat.py 2012-04-18 02:18:22 +0000
@@ -0,0 +1,38 @@
+from storm.locals import (
+ Bool,
+ DateTime,
+ Int,
+ )
+
+from lp.registry.enums import InformationType
+from lp.services.database.enumcol import EnumCol
+from lp.bugs.interfaces.bugtask import (
+ BugTaskImportance,
+ BugTaskStatus,
+ BugTaskStatusSearch,
+ )
+
+
+class BugTaskFlat(object):
+
+ __storm_table__ = 'BugTaskFlat'
+
+ bugtask_id = Int(name='bugtask', primary=True)
+ bug_id = Int(name='bug')
+ datecreated = DateTime()
+ duplicateof_id = Int(name='duplicateof')
+ bug_owner_id = Int(name='bug_owner')
+ information_type = EnumCol(enum=InformationType)
+ date_last_updated = DateTime()
+ heat = Int()
+ product_id = Int(name='product')
+ productseries_id = Int(name='productseries')
+ distribution_id = Int(name='distribution')
+ distroseries_id = Int(name='distroseries')
+ sourcepackagename_id = Int(name='sourcepackagename')
+ status = EnumCol(schema=(BugTaskStatus, BugTaskStatusSearch))
+ importance = EnumCol(schema=BugTaskImportance)
+ assignee_id = Int(name='assignee')
+ milestone_id = Int(name='milestone')
+ owner_id = Int(name='owner')
+ active = Bool()
=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py 2012-04-18 02:18:22 +0000
+++ lib/lp/bugs/model/bugtasksearch.py 2012-04-18 02:18:22 +0000
@@ -60,6 +60,7 @@
from lp.bugs.model.bugnomination import BugNomination
from lp.bugs.model.bugsubscription import BugSubscription
from lp.bugs.model.bugtask import BugTask
+from lp.bugs.model.bugtaskflat import BugTaskFlat
from lp.bugs.model.structuralsubscription import StructuralSubscription
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distroseries import IDistroSeries
@@ -71,6 +72,7 @@
from lp.registry.model.milestonetag import MilestoneTag
from lp.registry.model.person import Person
from lp.registry.model.product import Product
+from lp.services.database.bulk import load
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.lpstorm import IStore
from lp.services.database.sqlbase import (
@@ -81,6 +83,7 @@
Array,
NullCount,
)
+from lp.services.features import getFeatureFlag
from lp.services.propertycache import get_property_cache
from lp.services.searchbuilder import (
all,
@@ -241,39 +244,66 @@
respected.
:param just_bug_ids: Return a ResultSet of bug IDs instead of BugTasks.
"""
+ use_flat = bool(getFeatureFlag('bugs.bugtaskflat.search.enabled'))
+
store = IStore(BugTask)
- orderby_expression, orderby_joins = _process_order_by(alternatives[0])
+ orderby_expression, orderby_joins = _process_order_by(
+ alternatives[0], use_flat)
decorators = []
- want = BugTask.bugID if just_bug_ids else BugTask
+ # If we are to use BugTaskFlat, we just return the ID. The
+ # DecoratedResultSet will turn it into the actual BugTask.
+ # If we're not using BugTaskFlat yet, we should still return
+ # the BugTask directly.
+ if use_flat:
+ start = BugTaskFlat
+ if just_bug_ids:
+ want = BugTaskFlat.bug_id
+ else:
+ want = BugTaskFlat.bugtask_id
+ decorators.append(lambda id: IStore(BugTask).get(BugTask, id))
+ orig_pre_iter_hook = pre_iter_hook
+
+ def pre_iter_hook(rows):
+ rows = load(BugTask, rows)
+ if orig_pre_iter_hook:
+ orig_pre_iter_hook(rows)
+ else:
+ start = BugTask
+ want = BugTask.bugID if just_bug_ids else BugTask
if len(alternatives) == 1:
[query, clauseTables, bugtask_decorator, join_tables,
- has_duplicate_results, with_clause] = _build_query(alternatives[0])
+ has_duplicate_results, with_clause] = _build_query(
+ alternatives[0], use_flat)
if with_clause:
store = store.with_(with_clause)
decorators.append(bugtask_decorator)
if has_duplicate_results:
- origin = _build_origin(join_tables, clauseTables)
- outer_origin = _build_origin(orderby_joins, [])
- subquery = Select(BugTask.id, where=query, tables=origin)
+ origin = _build_origin(join_tables, clauseTables, start)
+ outer_origin = _build_origin(orderby_joins, [], start)
+ want_inner = BugTaskFlat.bugtask_id if use_flat else BugTask.id
+ subquery = Select(want_inner, where=query, tables=origin)
result = store.using(*outer_origin).find(
- want, In(BugTask.id, subquery))
+ want, In(want_inner, subquery))
else:
- origin = _build_origin(join_tables + orderby_joins, clauseTables)
+ origin = _build_origin(
+ join_tables + orderby_joins, clauseTables, start)
result = store.using(*origin).find(want, query)
else:
results = []
for params in alternatives:
[query, clauseTables, decorator, join_tables,
- has_duplicate_results, with_clause] = _build_query(params)
- origin = _build_origin(join_tables, clauseTables)
+ has_duplicate_results, with_clause] = _build_query(
+ params, use_flat)
+ origin = _build_origin(join_tables, clauseTables, start)
localstore = store
if with_clause:
localstore = store.with_(with_clause)
- next_result = localstore.using(*origin).find((BugTask,), query)
+ want_inner = BugTaskFlat if use_flat else BugTask
+ next_result = localstore.using(*origin).find((want_inner,), query)
results.append(next_result)
# NB: assumes the decorators are all compatible.
# This may need revisiting if e.g. searches on behalf of different
@@ -283,7 +313,9 @@
resultset = reduce(lambda l, r: l.union(r), results)
origin = _build_origin(
orderby_joins, [],
- start_with=Alias(resultset._get_select(), "BugTask"))
+ Alias(
+ resultset._get_select(),
+ "BugTaskFlat" if use_flat else "BugTask"))
result = store.using(*origin).find(want)
result.order_by(orderby_expression)
@@ -293,7 +325,7 @@
pre_iter_hook=pre_iter_hook)
-def _build_origin(join_tables, clauseTables, start_with=BugTask):
+def _build_origin(join_tables, clauseTables, start_with):
"""Build the parameter list for Store.using().
:param join_tables: A sequence of tables that should be joined
@@ -321,7 +353,7 @@
return origin
-def _build_query(params):
+def _build_query(params, use_flat):
"""Build and return an SQL query with the given parameters.
Also return the clauseTables and orderBy for the generated query.
@@ -331,9 +363,18 @@
"""
params = _require_params(params)
- extra_clauses = [Bug.id == BugTask.bugID]
- clauseTables = [BugTask, Bug]
- join_tables = []
+ if use_flat:
+ extra_clauses = []
+ clauseTables = []
+ join_tables = [
+ (Bug, Join(Bug, Bug.id == BugTaskFlat.bug_id)),
+ (BugTask, Join(BugTask, BugTask.id == BugTaskFlat.bugtask_id)),
+ ]
+ else:
+ extra_clauses = [Bug.id == BugTask.bugID]
+ clauseTables = [BugTask, Bug]
+ join_tables = []
+
decorators = []
has_duplicate_results = False
with_clauses = []
@@ -759,7 +800,7 @@
has_duplicate_results, with_clause)
-def _process_order_by(params):
+def _process_order_by(params, use_flat):
"""Process the orderby parameter supplied to search().
This method ensures the sort order will be stable, and converting
@@ -802,7 +843,13 @@
# Translate orderby keys into corresponding Table.attribute
# strings.
- extra_joins = []
+ if use_flat:
+ extra_joins = [
+ (Bug, Join(Bug, Bug.id == BugTaskFlat.bug_id)),
+ (BugTask, Join(BugTask, BugTask.id == BugTaskFlat.bugtask_id)),
+ ]
+ else:
+ extra_joins = []
ambiguous = True
# Sorting by milestone only is a very "coarse" sort order.
# If no additional sort order is specified, add the bug task
=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py 2012-04-17 08:01:35 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py 2012-04-18 02:18:22 +0000
@@ -31,6 +31,7 @@
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.services.features.testing import FeatureFixture
from lp.services.searchbuilder import (
all,
any,
@@ -1528,6 +1529,15 @@
return [bugtask.bug.id for bugtask in expected_bugtasks]
+class UsingFlat:
+ """Use BugTaskFlat for searching."""
+
+ def setUp(self):
+ super(UsingFlat, self).setUp()
+ self.useFixture(
+ FeatureFixture({'bugs.bugtaskflat.search.enabled': 'on'}))
+
+
class TestMilestoneDueDateFiltering(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
@@ -1563,17 +1573,21 @@
for bug_target_search_type_class in (
PreloadBugtaskTargets, NoPreloadBugtaskTargets, QueryBugIDs):
for target_mixin in bug_targets_mixins:
- class_name = 'Test%s%s' % (
- bug_target_search_type_class.__name__,
- target_mixin.__name__)
- class_bases = (
- target_mixin, bug_target_search_type_class,
- SearchTestBase, TestCaseWithFactory)
- # Dynamically build a test class from the target mixin class,
- # from the search type mixin class, from the mixin class
- # having all tests and from a unit test base class.
- test_class = type(class_name, class_bases, {})
- # Add the new unit test class to the suite.
- suite.addTest(loader.loadTestsFromTestCase(test_class))
+ for feature_mixin in (None, UsingFlat):
+ class_name = 'Test%s%s%s' % (
+ bug_target_search_type_class.__name__,
+ target_mixin.__name__,
+ feature_mixin.__name__ if feature_mixin else '')
+ mixins = [target_mixin, bug_target_search_type_class]
+ if feature_mixin:
+ mixins.append(feature_mixin)
+ class_bases = (
+ tuple(mixins) + (SearchTestBase, TestCaseWithFactory))
+ # Dynamically build a test class from the target mixin class,
+ # from the search type mixin class, from the mixin class
+ # having all tests and from a unit test base class.
+ test_class = type(class_name, class_bases, {})
+ # Add the new unit test class to the suite.
+ suite.addTest(loader.loadTestsFromTestCase(test_class))
suite.addTest(loader.loadTestsFromName(__name__))
return suite
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2012-04-11 14:34:28 +0000
+++ lib/lp/services/features/flags.py 2012-04-18 02:18:22 +0000
@@ -63,6 +63,12 @@
'',
'',
'https://bugs.launchpad.net/launchpad/+bug/678090'),
+ ('bugs.bugtaskflat.search.enabled',
+ 'boolean',
+ 'Use BugTaskFlat for bug searches.',
+ '',
+ '',
+ ''),
('bugs.bugtracker_components.enabled',
'boolean',
('Enables the display of bugtracker components.'),
