launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13461
[Merge] lp:~sinzui/launchpad/nominations-investigation-4 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/nominations-investigation-4 into lp:launchpad.
Commit message:
Cache the recipients when making repeated calls to send bug notifications.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #874250 in Launchpad itself: "BugNomination:+editstatus timeout for bugs with many tasks"
https://bugs.launchpad.net/launchpad/+bug/874250
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/nominations-investigation-4/+merge/129990
Bug.getBugNotificationRecipients() may be called multiple times when
creating notifications for multiple changes or bugs in many
relationships. In general, the method can only return three different
sets for most changes, so the queries are often repeated.
This change may not make bug nomination approval fast enough to
not timeout, but it will make Lp and the test suite faster in
general.
--------------------------------------------------------------------
RULES
Pre-implementation: wgrant, wallyworld
* Update bug.getBugNotificationRecipients to cache the RecipientSet
for the BugNotificationLevel that was passed to the method.
* Return a copy of the cached RecipientSet because many callsites
mutate the set.
* Ensure that actions that change which structural subscribers
are selected also clear the cache.
QA
* Clear the staging inbox.
* Ask webops to make you a member of
https://qastaging.launchpad.net/~ubuntu-release
* Visit https://bugs.qastaging.launchpad.net/gdp/+bug/939614
and use the +editstaus form to change a task's milestone to 0.5.12.
* Verify a notification was sent to Curtis.
* Visit https://bugs.qastaging.launchpad.net/gdp/+bug/939614
and use the +editstaus form to change a task's milestone to 0.5.13.
* Verify a notification was sent to Caroline.
* Visit https://bugs.qastaging.launchpad.net/gdp/+bug/939614
and use the +editstaus form to change the project to pocket-lint.
* Verify a notification was sent to the me-9 team (check headers).
* Visit https://bugs.qastaging.launchpad.net/ubuntu/+source/obexd/+bug/872044
* Verify the page does not timeout...it will timeout trying to load the
first time so if this works, the escalated bug could be fixed.
LINT
lib/lp/bugs/doc/bugsubscription.txt
lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/model/tests/test_bugtask.py
lib/lp/bugs/tests/test_bug_notification_recipients.py
lib/lp/bugs/tests/test_bugchanges.py
Loc
This branch adds about 150 lines, but I have a credit of 4,900 lines
as of this week. If you insist, I can gut doc/bugsubscription.txt
because I suspect it duplicates unit tests.
TEST
./bin/test -vvc -t cache lp.bugs.tests.test_bug_notification_recipients
./bin/test -vvc -t cached lp.bugs.model.tests.test_bugtask
./bin/test -vvc \
-t doc/bugnotificationrecipients.txt -t doc/bugsubscription.txt \
-t test_bug_notification_recipients -t bugs-emailinterface.txt \
-t test_bugchanges lp.bugs
IMPLEMENTATION
I changed getBugNotificationRecipients to be a private method that does
the real work. I added a replacement method that differs to
cachedproperties and returns a new BugNotificationRecipients set. I
added a method to clear the caches.
lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/tests/test_bug_notification_recipients.py
I updated the transition methods that change the affected structures to
clear the caches.
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/model/tests/test_bugtask.py
I updated tests confused by the cache changes.
lib/lp/bugs/doc/bugsubscription.txt
lib/lp/bugs/tests/test_bugchanges.py
--
https://code.launchpad.net/~sinzui/launchpad/nominations-investigation-4/+merge/129990
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/nominations-investigation-4 into lp:launchpad.
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2012-09-17 16:13:40 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2012-10-16 21:28:23 +0000
@@ -748,6 +748,7 @@
... for subscription in new_bug.subscriptions]
[u'Foo Bar']
+ >>> new_bug.clearBugNotificationRecipientsCache()
>>> getSubscribers(new_bug)
['foo.bar@xxxxxxxxxxxxx', 'support@xxxxxxxxxx']
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2012-10-15 20:52:21 +0000
+++ lib/lp/bugs/interfaces/bug.py 2012-10-16 21:28:23 +0000
@@ -488,6 +488,14 @@
details of this implementation.
"""
+ def clearBugNotificationRecipientsCache():
+ """Clear the bug notification recipient BugNotificationLevel cache.
+
+ Call this when a change to a bug or bugtask would change the
+ notification recipients. Changing a a bugtask's milestone or
+ target is such a case.
+ """
+
def canBeAQuestion():
"""Return True of False if a question can be created from this bug.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-10-15 20:52:21 +0000
+++ lib/lp/bugs/model/bug.py 2012-10-16 21:28:23 +0000
@@ -1085,15 +1085,53 @@
"""
return get_also_notified_subscribers(self, recipients, level)
- def getBugNotificationRecipients(self,
- level=BugNotificationLevel.LIFECYCLE):
- """See `IBug`."""
+ def _getBugNotificationRecipients(self, level):
+ """Get the recipients for the BugNotificationLevel."""
recipients = BugNotificationRecipients()
self.getDirectSubscribers(
recipients, level=level, filter_visible=True)
self.getIndirectSubscribers(recipients, level=level)
return recipients
+ @cachedproperty
+ def _notification_recipients_for_lifecycle(self):
+ """The cached BugNotificationRecipients for LIFECYCLE events."""
+ return self._getBugNotificationRecipients(
+ BugNotificationLevel.LIFECYCLE)
+
+ @cachedproperty
+ def _notification_recipients_for_metadata(self):
+ """The cached BugNotificationRecipients for METADATA events."""
+ return self._getBugNotificationRecipients(
+ BugNotificationLevel.METADATA)
+
+ @cachedproperty
+ def _notification_recipients_for_comments(self):
+ """The cached BugNotificationRecipients for COMMENT events."""
+ return self._getBugNotificationRecipients(
+ BugNotificationLevel.COMMENTS)
+
+ def getBugNotificationRecipients(self,
+ level=BugNotificationLevel.LIFECYCLE):
+ """See `IBug`."""
+ recipients = BugNotificationRecipients()
+ if level == BugNotificationLevel.LIFECYCLE:
+ recipients.update(self._notification_recipients_for_lifecycle)
+ elif level == BugNotificationLevel.METADATA:
+ recipients.update(self._notification_recipients_for_metadata)
+ else:
+ recipients.update(self._notification_recipients_for_comments)
+ return recipients
+
+ def clearBugNotificationRecipientsCache(self):
+ cache = get_property_cache(self)
+ if getattr(cache, '_notification_recipients_for_lifecycle', False):
+ del cache._notification_recipients_for_lifecycle
+ if getattr(cache, '_notification_recipients_for_metadata', False):
+ del cache._notification_recipients_for_metadata
+ if getattr(cache, '_notification_recipients_for_comments', False):
+ del cache._notification_recipients_for_comments
+
def addCommentNotification(self, message, recipients=None, activity=None):
"""See `IBug`."""
if recipients is None:
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2012-10-15 02:32:30 +0000
+++ lib/lp/bugs/model/bugtask.py 2012-10-16 21:28:23 +0000
@@ -772,8 +772,10 @@
raise UserCannotEditBugTaskMilestone(
"User does not have sufficient permissions "
"to edit the bug task milestone.")
- else:
- self.milestone = new_milestone
+ self.milestone = new_milestone
+ # Clear the recipient caches so that the milestone subscribers are
+ # notified.
+ self.bug.clearBugNotificationRecipientsCache()
def transitionToImportance(self, new_importance, user):
"""See `IBugTask`."""
@@ -1141,6 +1143,7 @@
# As a result of the transition, some subscribers may no longer
# have access to the parent bug. We need to run a job to remove any
# such subscriptions.
+ self.bug.clearBugNotificationRecipientsCache()
getUtility(IRemoveArtifactSubscriptionsJobSource).create(
user, [self.bug], pillar=target_before_change.pillar)
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2012-09-27 14:25:32 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2012-10-16 21:28:23 +0000
@@ -86,6 +86,7 @@
from lp.services.features.testing import FeatureFixture
from lp.services.job.tests import block_on_job
from lp.services.log.logger import FakeLogger
+from lp.services.propertycache import get_property_cache
from lp.services.searchbuilder import any
from lp.services.webapp.authorization import check_permission
from lp.services.webapp.interfaces import ILaunchBag
@@ -2634,6 +2635,20 @@
new_product.bugtargetdisplayname,
removeSecurityProxy(task).targetnamecache)
+ def test_cached_recipients_cleared(self):
+ # The bug's notification recipients caches are cleared when
+ # transitionToTarget() is called.
+ new_product = self.factory.makeProduct()
+ task = self.factory.makeBugTask()
+ # The factory caused COMMENT notifications which filled the bug cache.
+ cache = get_property_cache(task.bug)
+ self.assertIsNotNone(
+ getattr(cache, '_notification_recipients_for_comments', None))
+ with person_logged_in(task.owner):
+ task.transitionToTarget(new_product, task.owner)
+ self.assertIsNone(
+ getattr(cache, '_notification_recipients_for_comments', None))
+
def test_accesspolicyartifacts_updated(self):
# transitionToTarget updates the AccessPolicyArtifacts related
# to the bug.
@@ -2673,6 +2688,27 @@
[sp, sp.distribution_sourcepackage, other_distro])
+class TransitionToMilestoneTestCase(TestCaseWithFactory):
+ """Tests for BugTask.transitionToMilestone."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_cached_recipients_cleared(self):
+ # The bug's notification recipients caches are cleared when
+ # transitionToMilestone() is called.
+ task = self.factory.makeBugTask()
+ product = task.target
+ milestone = self.factory.makeMilestone(product=product)
+ # The factory caused COMMENT notifications which filled the bug cache.
+ cache = get_property_cache(task.bug)
+ self.assertIsNotNone(
+ getattr(cache, '_notification_recipients_for_comments', None))
+ with person_logged_in(task.target.owner):
+ task.transitionToMilestone(milestone, task.target.owner)
+ self.assertIsNone(
+ getattr(cache, '_notification_recipients_for_comments', None))
+
+
class TestTransitionsRemovesSubscribersJob(TestCaseWithFactory):
"""Test that various bug transitions invoke RemoveArtifactSubscribers
job."""
=== modified file 'lib/lp/bugs/tests/test_bug_notification_recipients.py'
--- lib/lp/bugs/tests/test_bug_notification_recipients.py 2012-09-18 18:36:09 +0000
+++ lib/lp/bugs/tests/test_bug_notification_recipients.py 2012-10-16 21:28:23 +0000
@@ -6,17 +6,25 @@
__metaclass__ = type
from zope.component import getUtility
+from testtools.matchers import (
+ Equals,
+ GreaterThan,
+ )
from lp.app.enums import InformationType
+from lp.bugs.enums import BugNotificationLevel
from lp.registry.interfaces.accesspolicy import (
IAccessArtifactGrantSource,
IAccessPolicySource,
)
+from lp.services.propertycache import get_property_cache
from lp.testing import (
person_logged_in,
+ StormStatementRecorder,
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
class TestBugNotificationRecipients(TestCaseWithFactory):
@@ -185,3 +193,57 @@
dupe.markAsDuplicate(bug)
self.assertContentEqual(
[owner, subscriber], bug.getBugNotificationRecipients())
+
+ def test_cache_by_bug_notification_level(self):
+ # The BugNotificationRecipients set is cached by notification level
+ # to avoid duplicate work. The returned set is copy of the cached set.
+ subscriber = self.factory.makePerson()
+ product = self.factory.makeProduct()
+ with person_logged_in(subscriber):
+ subscription = product.addBugSubscription(subscriber, subscriber)
+ bug_filter = subscription.bug_filters[0]
+ bug_filter.bug_notification_level = BugNotificationLevel.COMMENTS
+ bug = self.factory.makeBug(target=product)
+ # The factory call queued LIFECYCLE and COMMENT notifications.
+ bug.clearBugNotificationRecipientsCache()
+ levels = [
+ BugNotificationLevel.LIFECYCLE,
+ BugNotificationLevel.METADATA,
+ BugNotificationLevel.COMMENTS,
+ ]
+ for level in levels:
+ with StormStatementRecorder() as recorder:
+ first_recipients = bug.getBugNotificationRecipients(
+ level=level)
+ self.assertThat(recorder, HasQueryCount(GreaterThan(1)))
+ with StormStatementRecorder() as recorder:
+ second_recipients = bug.getBugNotificationRecipients(
+ level=level)
+ self.assertThat(recorder, HasQueryCount(Equals(0)))
+ self.assertContentEqual([bug.owner, subscriber], first_recipients)
+ self.assertContentEqual(first_recipients, second_recipients)
+ self.assertIsNot(first_recipients, second_recipients)
+
+ def test_clearBugNotificationRecipientCache(self):
+ subscriber = self.factory.makePerson()
+ product = self.factory.makeProduct()
+ with person_logged_in(subscriber):
+ subscription = product.addBugSubscription(subscriber, subscriber)
+ bug_filter = subscription.bug_filters[0]
+ bug_filter.bug_notification_level = BugNotificationLevel.COMMENTS
+ bug = self.factory.makeBug(target=product)
+ levels = [
+ BugNotificationLevel.LIFECYCLE,
+ BugNotificationLevel.METADATA,
+ BugNotificationLevel.COMMENTS,
+ ]
+ for level in levels:
+ bug.getBugNotificationRecipients(level=level)
+ bug.clearBugNotificationRecipientsCache()
+ cache = get_property_cache(bug)
+ self.assertIsNone(
+ getattr(cache, '_notification_recipients_for_lifecycle', None))
+ self.assertIsNone(
+ getattr(cache, '_notification_recipients_for_metadata', None))
+ self.assertIsNone(
+ getattr(cache, '_notification_recipients_for_comments', None))
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py 2012-10-12 03:11:38 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py 2012-10-16 21:28:23 +0000
@@ -95,6 +95,7 @@
else:
self.old_activities = old_activities
self.old_notification_ids = old_notification_ids
+ bug.clearBugNotificationRecipientsCache()
def changeAttribute(self, obj, attribute, new_value):
"""Set the value of `attribute` on `obj` to `new_value`.
Follow ups
