launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13699
[Merge] lp:~wallyworld/launchpad/bug-distrotask-timeout-1012309 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-distrotask-timeout-1012309 into lp:launchpad.
Commit message:
Improve bug form and model infrastructure to avoid repeated calls to various validation methods.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1012309 in Launchpad itself: "BugTask:+distrotask timeout adding a distrotask to a new bug"
https://bugs.launchpad.net/launchpad/+bug/1012309
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-distrotask-timeout-1012309/+merge/131308
== Implementation ==
Timeouts occurred add a new bug target or editing an existing target.
The timeout was due to repeated invocations of a query on the source package tables. The call point for the query was the guessPublishedSourcePackageName() method on distribution. The root cause was twofold:
1. The bug target widget override _toFieldValue() and the zope form infrastructure calls this method repeatedly during a form submission. Sadly, this method calls guessPublishedSourcePackageName() because it doesn't trust the vocab used for the source package.
2. The form submission and model processing of the new or updated bug target validates the new bug target multiple times, as well as all existing affected targets for the other bug tasks. Again, guessPublishedSourcePackageName() is called each time.
The next effect was that a query taking 100's of ms or more on production could be run 8 or more times depending on how many bugtasks were on the bug.
The fix had multiple parts.
1. Add flags to various bugtask methods to allow validation to be bypassed (default = do validation).
2. Cache the field input -> value mapping in the bug task widget so that when it is called many times by the form infrastructure, it is cheap to do.
I also noticed a curious implementation decision on the LaunchpadTarget widget. The error() method was overridden and instead of using self._error (which is meant to be set during the field input processing), it called getInputValue() all over again and caught the raised exception. Not smart, especially when getInputValue() hits the database etc. So I fixed it all up.
== Tests ==
Internal changes, so rely on existing tests.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/widgets/launchpadtarget.py
lib/lp/bugs/browser/bugalsoaffects.py
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/widgets/bugtask.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/bugtask.py
--
https://code.launchpad.net/~wallyworld/launchpad/bug-distrotask-timeout-1012309/+merge/131308
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-distrotask-timeout-1012309 into lp:launchpad.
=== modified file 'lib/lp/app/widgets/launchpadtarget.py'
--- lib/lp/app/widgets/launchpadtarget.py 2012-10-04 23:15:35 +0000
+++ lib/lp/app/widgets/launchpadtarget.py 2012-10-25 05:17:04 +0000
@@ -110,35 +110,31 @@
try:
return self.product_widget.getInputValue()
except MissingInputError:
- raise LaunchpadValidationError('Please enter a project name')
+ self._error = LaunchpadValidationError(
+ 'Please enter a project name')
+ raise self._error
except ConversionError:
entered_name = self.request.form_ng.getOne(
"%s.product" % self.name)
- raise LaunchpadValidationError(
+ self._error = LaunchpadValidationError(
"There is no project named '%s' registered in"
" Launchpad" % entered_name)
+ raise self._error
elif form_value == 'package':
try:
distribution = self.distribution_widget.getInputValue()
except ConversionError:
entered_name = self.request.form_ng.getOne(
"%s.distribution" % self.name)
- raise LaunchpadValidationError(
+ self._error = LaunchpadValidationError(
"There is no distribution named '%s' registered in"
" Launchpad" % entered_name)
-
+ raise self._error
if self.package_widget.hasInput():
try:
package_name = self.package_widget.getInputValue()
- except ConversionError:
- entered_name = self.request.form_ng.getOne(
- '%s.package' % self.name)
- raise LaunchpadValidationError(
- "There is no package named '%s' published in %s."
- % (entered_name, distribution.displayname))
- if package_name is None:
- return distribution
- try:
+ if package_name is None:
+ return distribution
if IDistributionSourcePackage.providedBy(package_name):
dsp = package_name
else:
@@ -146,10 +142,13 @@
distribution.guessPublishedSourcePackageName(
package_name.name))
dsp = distribution.getSourcePackage(source_name)
- except NotFoundError:
- raise LaunchpadValidationError(
+ except (ConversionError, NotFoundError):
+ entered_name = self.request.form_ng.getOne(
+ '%s.package' % self.name)
+ self._error = LaunchpadValidationError(
"There is no package named '%s' published in %s."
- % (package_name.name, distribution.displayname))
+ % (entered_name, distribution.displayname))
+ raise self._error
return dsp
else:
return distribution
@@ -172,15 +171,6 @@
else:
raise AssertionError('Not a valid value: %r' % value)
- def error(self):
- """See zope.app.form.interfaces.IBrowserWidget."""
- try:
- if self.hasInput():
- self.getInputValue()
- except InputErrors as error:
- self._error = error
- return super(LaunchpadTargetWidget, self).error()
-
def __call__(self):
"""See zope.app.form.interfaces.IBrowserWidget."""
self.setUpSubWidgets()
=== modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
--- lib/lp/bugs/browser/bugalsoaffects.py 2012-10-04 23:15:35 +0000
+++ lib/lp/bugs/browser/bugalsoaffects.py 2012-10-25 05:17:04 +0000
@@ -285,13 +285,10 @@
else:
task_target = data['distribution']
if data.get('sourcepackagename') is not None:
- spn_or_dsp = data['sourcepackagename']
- if IDistributionSourcePackage.providedBy(spn_or_dsp):
- task_target = spn_or_dsp
- else:
- task_target = task_target.getSourcePackage(spn_or_dsp)
+ task_target = data['sourcepackagename']
+ # The new target has already been validated so don't do it again.
self.task_added = self.context.bug.addTask(
- getUtility(ILaunchBag).user, task_target)
+ getUtility(ILaunchBag).user, task_target, validate_target=False)
task_added = self.task_added
if extracted_bug is not None:
@@ -450,6 +447,8 @@
if sourcepackagename:
target = target.getSourcePackage(sourcepackagename)
validate_new_target(self.context.bug, target)
+ if sourcepackagename:
+ data['sourcepackagename'] = target
except IllegalTarget as e:
if sourcepackagename:
self.setFieldError('sourcepackagename', e[0])
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2012-10-15 02:32:30 +0000
+++ lib/lp/bugs/browser/bugtask.py 2012-10-25 05:17:04 +0000
@@ -240,7 +240,6 @@
from lp.registry.interfaces.projectgroup import IProjectGroup
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.model.personroles import PersonRoles
-from lp.registry.vocabularies import MilestoneVocabulary
from lp.services.config import config
from lp.services.features import getFeatureFlag
from lp.services.feeds.browser import (
@@ -1554,7 +1553,10 @@
new_target = data.get('target')
if new_target and new_target != self.context.target:
try:
- self.context.validateTransitionToTarget(new_target)
+ # The validity of the source package has already been checked
+ # by the bug target widget.
+ self.context.validateTransitionToTarget(
+ new_target, check_source_package=False)
except IllegalTarget as e:
self.setFieldError(error_field, e[0])
@@ -1635,9 +1637,10 @@
# guaranteed to pass all the values. For example: bugtasks linked to a
# bug watch don't allow editing the form, and the value is missing
# from the form.
+ # The new target has already been validated so don't do it again.
if new_target is not missing and bugtask.target != new_target:
changed = True
- bugtask.transitionToTarget(new_target, self.user)
+ bugtask.transitionToTarget(new_target, self.user, validate=False)
# Now that we've updated the bugtask we can add messages about
# milestone changes, if there were any.
=== modified file 'lib/lp/bugs/browser/widgets/bugtask.py'
--- lib/lp/bugs/browser/widgets/bugtask.py 2012-10-05 04:11:31 +0000
+++ lib/lp/bugs/browser/widgets/bugtask.py 2012-10-25 05:17:04 +0000
@@ -487,6 +487,8 @@
It accepts both binary and source package names.
"""
+ cached_values = {}
+
def getDistribution(self):
"""Get the distribution used for package validation.
@@ -506,7 +508,9 @@
return self.context.missing_value
distribution = self.getDistribution()
-
+ cached_value = self.cached_values.get(input)
+ if cached_value:
+ return cached_value
try:
source = distribution.guessPublishedSourcePackageName(input)
except NotFoundError:
@@ -516,6 +520,7 @@
raise ConversionError(
"Launchpad doesn't know of any source package named"
" '%s' in %s." % (input, distribution.displayname))
+ self.cached_values[input] = source
return source
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-10-18 21:24:32 +0000
+++ lib/lp/bugs/model/bug.py 2012-10-25 05:17:04 +0000
@@ -193,7 +193,6 @@
from lp.registry.model.pillar import pillar_sort_key
from lp.registry.model.teammembership import TeamParticipation
from lp.services.config import config
-from lp.services.database import bulk
from lp.services.database.constants import UTC_NOW
from lp.services.database.datetimecol import UtcDateTimeCol
from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -1247,9 +1246,10 @@
Store.of(result).flush()
return result
- def addTask(self, owner, target):
+ def addTask(self, owner, target, validate_target=True):
"""See `IBug`."""
- return getUtility(IBugTaskSet).createTask(self, owner, target)
+ return getUtility(IBugTaskSet).createTask(
+ self, owner, target, validate_target)
def addWatch(self, bugtracker, remotebug, owner):
"""See `IBug`."""
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2012-10-19 15:11:20 +0000
+++ lib/lp/bugs/model/bugtask.py 2012-10-25 05:17:04 +0000
@@ -314,10 +314,17 @@
return validate_person(self, attr, value)
-def validate_target(bug, target, retarget_existing=True):
+def validate_target(bug, target, retarget_existing=True,
+ check_source_package=True):
"""Validate a bugtask target against a bug's existing tasks.
Checks that no conflicting tasks already exist.
+
+ If the target is a source package, we need to check that it has been
+ published in the distribution since we don't trust the vocabulary to
+ enforce this. However, when using the UI, this check is done during the
+ validation stage of form submission and we don't want to do it again since
+ it uses an expensive query. So 'check_source_package' can be set to False.
"""
if bug.getBugTask(target):
raise IllegalTarget(
@@ -328,7 +335,7 @@
ISourcePackage.providedBy(target)):
# If the distribution has at least one series, check that the
# source package has been published in the distribution.
- if (target.sourcepackagename is not None and
+ if (check_source_package and target.sourcepackagename is not None and
len(target.distribution.series) > 0):
try:
target.distribution.guessPublishedSourcePackageName(
@@ -664,10 +671,11 @@
if relevant:
key = bug_target_to_key(bugtask.target)
key['sourcepackagename'] = new_spn
+ # The relevance check above and the fact that the distro series
+ # task is already on the bug means we don't need to revalidate.
bugtask.transitionToTarget(
bug_target_from_key(**key),
- user,
- _sync_sourcepackages=False)
+ user, validate=False, _sync_sourcepackages=False)
def getContributorInfo(self, user, person):
"""See `IBugTask`."""
@@ -1035,7 +1043,7 @@
self.assignee = assignee
- def validateTransitionToTarget(self, target):
+ def validateTransitionToTarget(self, target, check_source_package=True):
"""See `IBugTask`."""
from lp.registry.model.distroseries import DistroSeries
@@ -1094,11 +1102,17 @@
"tasks may only be retargeted to a different "
"package.")
- validate_target(self.bug, target)
+ validate_target(
+ self.bug, target, check_source_package=check_source_package)
- def transitionToTarget(self, target, user, _sync_sourcepackages=True):
+ def transitionToTarget(self, target, user, validate=True,
+ _sync_sourcepackages=True):
"""See `IBugTask`.
+ If validate is True then we need to check that the new target is valid,
+ otherwise the check has already been done (eg during form submission)
+ and we don't need to repeat it.
+
If _sync_sourcepackages is True (the default) and the
sourcepackagename is being changed, any other tasks for the same
name in this distribution will have their names updated to
@@ -1107,7 +1121,8 @@
if self.target == target:
return
- self.validateTransitionToTarget(target)
+ if validate:
+ self.validateTransitionToTarget(target)
target_before_change = self.target
@@ -1570,8 +1585,9 @@
params = BugTaskSearchParams(user, **kwargs)
return self.search(params)
- def createManyTasks(self, bug, owner, targets, status=None,
- importance=None, assignee=None, milestone=None):
+ def createManyTasks(self, bug, owner, targets, validate_target=True,
+ status=None, importance=None, assignee=None,
+ milestone=None):
"""See `IBugTaskSet`."""
if status is None:
status = IBugTask['status'].default
@@ -1580,7 +1596,8 @@
target_keys = []
pillars = set()
for target in targets:
- validate_new_target(bug, target)
+ if validate_target:
+ validate_new_target(bug, target)
pillars.add(target.pillar)
target_keys.append(bug_target_to_key(target))
@@ -1605,8 +1622,8 @@
removeSecurityProxy(bug)._reconcileAccess()
return tasks
- def createTask(self, bug, owner, target, status=None, importance=None,
- assignee=None, milestone=None):
+ def createTask(self, bug, owner, target, validate_target=True, status=None,
+ importance=None, assignee=None, milestone=None):
"""See `IBugTaskSet`."""
# Create tasks for accepted nominations if this is a source
# package addition. Distribution nominations are for all the
@@ -1622,8 +1639,9 @@
key['sourcepackagename']))
tasks = self.createManyTasks(
- bug, owner, targets, status=status, importance=importance,
- assignee=assignee, milestone=milestone)
+ bug, owner, targets, validate_target=validate_target,
+ status=status, importance=importance, assignee=assignee,
+ milestone=milestone)
return [task for task in tasks if task.target == target][0]
def getStatusCountsForProductSeries(self, user, product_series):
Follow ups
