Merge lp:~gmb/launchpad/hook-up-stored-proc-boom-bug-585803 into lp:launchpad/db-devel

[Graham Binns]

Bug heat is now updated directly rather than relying on CalculateBugHeatJob creation.

This branch converts all current sites where CalculateBugHeatJobs are created to use IBug.updateHeat() or to call the calculate_bug_heat() stored procedure directly.

I've removed all CalculateBugHeatJob creation callsites (except in tests, which I'll remove in a subsequent branch along with all the CalculateBugHeatJob infrastructure) and I've updated the BugHeatUpdater garbo job to call the calculate_bug_heat() stored procedure directly rather than looping over out-of-date bugs.

I've also removed the tests that covered the callsites that I've removed.

[Abel Deuring (adeuring)]

Hi Graham,

we discussed that it would be better to update the heat values in chunks, with initial chunk size of 1000. I like your implementation as in http://pastebin.ubuntu.com/440470/

thanks for finally fixing the heat updater!

review: Approve (code)

[Curtis Hovey (sinzui)]

This is good to land and you have my RC to ensure this landed even if ec2 runs slowly.

review: Approve (rc)

=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql	2010-05-29 16:21:26 +0000
+++ database/schema/trusted.sql	2010-05-29 16:21:27 +0000
@@ -1696,7 +1696,8 @@
         days_since_created = (datetime.utcnow() - date_created).days
         max_heat = get_max_heat_for_bug(bug_id)
         if max_heat is not None and days_since_created > 0:
-            return total_heat + (max_heat * 0.25 / days_since_created)
+            total_heat = (
+                total_heat + (max_heat * 0.25 / days_since_created))
 
     return int(total_heat)
 $$;
 
=== added directory 'lib/canonical/launchpad/apidoc'
=== modified file 'lib/canonical/launchpad/scripts/garbo.py'
--- lib/canonical/launchpad/scripts/garbo.py	2010-04-14 13:14:00 +0000
+++ lib/canonical/launchpad/scripts/garbo.py	2010-05-29 16:21:27 +0000
@@ -13,6 +13,7 @@
 import transaction
 from psycopg2 import IntegrityError
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 from storm.locals import In, SQL, Max, Min
 
 from canonical.config import config
@@ -30,6 +31,7 @@
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
 from lp.bugs.interfaces.bug import IBugSet
+from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugattachment import BugAttachment
 from lp.bugs.interfaces.bugjob import ICalculateBugHeatJobSource
 from lp.bugs.model.bugnotification import BugNotification
@@ -536,50 +538,39 @@
             max_heat_age = config.calculate_bug_heat.max_heat_age
         self.max_heat_age = max_heat_age
 
+        self.store = IMasterStore(Bug)
+
+    @property
+    def _outdated_bugs(self):
+        outdated_bugs = getUtility(IBugSet).getBugsWithOutdatedHeat(
+            self.max_heat_age)
+        # We remove the security proxy so that we can access the set()
+        # method of the result set.
+        return removeSecurityProxy(outdated_bugs)
+
     def isDone(self):
         """See `ITunableLoop`."""
         # When the main loop has no more Bugs to process it sets
         # offset to None. Until then, it always has a numerical
         # value.
-        return self.is_done
+        return self._outdated_bugs.is_empty()
 
     def __call__(self, chunk_size):
         """Retrieve a batch of Bugs and update their heat.
 
         See `ITunableLoop`.
         """
-        # XXX 2010-01-08 gmb bug=198767:
-        #     We cast chunk_size to an integer to ensure that we're not
-        #     trying to slice using floats or anything similarly
-        #     foolish. We shouldn't have to do this.
-        chunk_size = int(chunk_size)
-        start = self.offset
-        end = self.offset + chunk_size
+        # We multiply chunk_size by 1000 for the sake of doing updates
+        # quickly.
+        chunk_size = int(chunk_size * 1000)
 
         transaction.begin()
-        bugs = getUtility(IBugSet).getBugsWithOutdatedHeat(
-            self.max_heat_age)[start:end]
-
-        bug_count = bugs.count()
-        if bug_count > 0:
-            starting_id = bugs.first().id
-            self.log.debug(
-                "Adding CalculateBugHeatJobs for %i Bugs (starting id: %i)" %
-                (bug_count, starting_id))
-        else:
-            self.is_done = True
-
-        self.offset = None
-        for bug in bugs:
-            # We set the starting point of the next batch to the Bug
-            # id after the one we're looking at now. If there aren't any
-            # bugs this loop will run for 0 iterations and starting_id
-            # will remain set to None.
-            start += 1
-            self.offset = start
-            self.log.debug("Adding CalculateBugHeatJob for bug %s" % bug.id)
-            getUtility(ICalculateBugHeatJobSource).create(bug)
-            self.total_processed += 1
+        outdated_bugs = self._outdated_bugs[:chunk_size]
+        self.log.debug("Updating heat for %s bugs" % outdated_bugs.count())
+        outdated_bugs.set(
+            heat=SQL('calculate_bug_heat(Bug.id)'),
+            heat_last_updated=datetime.now(pytz.utc))
+
         transaction.commit()
 
 
 
=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt	2010-05-29 16:21:26 +0000
+++ lib/lp/bugs/doc/bug-heat.txt	2010-05-29 16:21:27 +0000
@@ -62,15 +62,11 @@
     6
 
 
-Adjusting bug heat in transaction
----------------------------------
-
-Sometimes, when a bug changes, we want to see the changes reflected in
-the bug's heat value immediately, without waiting for heat to be
-recalculated. Currently we adjust heat immediately for bug privacy and
-security.
-
-    >>> from canonical.database.sqlbase import flush_database_updates
+Events which trigger bug heat updates
+-------------------------------------
+
+There are several events which will cause a bug's heat to be updated.
+First, as stated above, heat will be calculated when the bug is created.
 
     >>> bug = factory.makeBug(owner=bug_owner)
     >>> bug.heat
@@ -80,18 +76,116 @@
 product against which it's filed, whose subscription is converted from
 an indirect to a direct subscription.
 
+Marking a bug as private also gives it an extra 150 heat points.
+
     >>> changed = bug.setPrivate(True, bug_owner)
     >>> bug.heat
     158
 
+Setting the bug as security related adds another 250 heat points.
+
     >>> changed = bug.setSecurityRelated(True)
     >>> bug.heat
     408
 
+Marking the bug public removes 150 heat points.
+
     >>> changed = bug.setPrivate(False, bug_owner)
     >>> bug.heat
     258
 
+And marking it not security-related removes 250 points.
+
+    >>> changed = bug.setSecurityRelated(False)
+    >>> bug.heat
+    8
+
+Adding a subscriber to the bug increases its heat by 2 points.
+
+    >>> new_subscriber = factory.makePerson()
+    >>> subscription = bug.subscribe(new_subscriber, new_subscriber)
+    >>> bug.heat
+    10
+
+When a user unsubscribes, the bug loses 2 points of heat.
+
+    >>> bug.unsubscribe(new_subscriber, new_subscriber)
+    >>> bug.heat
+    8
+
+Should a user mark themselves as affected by the bug, it will gain 4
+points of heat.
+
+    >>> bug.markUserAffected(new_subscriber)
+    >>> bug.heat
+    12
+
+If a user who was previously affected marks themself as not affected,
+the bug loses 4 points of heat.
+
+    >>> bug.markUserAffected(new_subscriber, False)
+    >>> bug.heat
+    8
+
+If a user who wasn't affected by the bug marks themselve as explicitly
+unaffected, the bug's heat doesn't change.
+
+    >>> unaffected_person = factory.makePerson()
+    >>> bug.markUserAffected(unaffected_person, False)
+    >>> bug.heat
+    8
+
+Marking the bug as a duplicate will set its heat to zero, whilst also
+adding 10 points of heat to the bug it duplicates, 6 points for the
+duplication and 4 points for the subscribers that the duplicated bug
+inherits.
+
+    >>> duplicated_bug = factory.makeBug()
+    >>> duplicated_bug.heat
+    6
+
+    >>> bug.markAsDuplicate(duplicated_bug)
+    >>> bug.heat
+    0
+
+    >>> duplicated_bug.heat
+    16
+
+Unmarking the bug as a duplicate restores its heat and updates the
+duplicated bug's heat.
+
+    >>> bug.markAsDuplicate(None)
+    >>> bug.heat
+    8
+
+    >>> duplicated_bug.heat
+    6
+
+A number of other changes, handled by the Bug's addChange() method, will
+cause heat to be recalculated, even if the heat itself may not actually
+change.
+
+For example, updating the bug's description calls the addChange() event,
+and will cause the bug's heat to be recalculated.
+
+We'll set the bug's heat to 0 first to demonstrate this.
+
+    >>> bug.setHeat(0)
+    >>> bug.heat
+    0
+
+    >>> from datetime import datetime, timedelta
+    >>> from pytz import timezone
+
+    >>> from lp.bugs.adapters.bugchange import BugDescriptionChange
+    >>> change = BugDescriptionChange(
+    ...     when=datetime.now().replace(tzinfo=timezone('UTC')),
+    ...     person=bug.owner, what_changed='description',
+    ...     old_value=bug.description, new_value='Some text')
+    >>> bug.addChange(change)
+    >>> bug.heat
+    8
+
 
 Getting bugs whose heat is outdated
 -----------------------------------
@@ -122,8 +216,6 @@
 getBugsWithOutdatedHeat() it will appear in the set returned by
 getBugsWithOutdatedHeat().
 
-    >>> from datetime import datetime, timedelta
-    >>> from pytz import timezone
     >>> old_heat_bug = factory.makeBug()
     >>> old_heat_bug.setHeat(
     ...     0, datetime.now(timezone('UTC')) - timedelta(days=2))
@@ -161,52 +253,24 @@
     >>> from canonical.launchpad.scripts.garbo import BugHeatUpdater
     >>> from canonical.launchpad.scripts import FakeLogger
 
+We'll commit the transaction so that the BugHeatUpdater updates the
+right bugs.
+
+    >>> transaction.commit()
     >>> update_bug_heat = BugHeatUpdater(FakeLogger(), max_heat_age=1)
 
 BugHeatUpdater implements ITunableLoop and as such is callable. Calling
-it as a method will add jobs to calculate the heat of for all the bugs
-whose heat is more than seven days old.
-
-Before update_bug_heat is called, we'll ensure that there are no waiting
-jobs in the bug heat calculation queue.
-
-    >>> from lp.bugs.interfaces.bugjob import ICalculateBugHeatJobSource
-    >>> for calc_job in getUtility(ICalculateBugHeatJobSource).iterReady():
-    ...     calc_job.job.start()
-    ...     calc_job.job.complete()
-
-    >>> ready_jobs = list(getUtility(ICalculateBugHeatJobSource).iterReady())
-    >>> len(ready_jobs)
-    0
-
-We need to commit here to ensure that the bugs we've created are
-available to the update_bug_heat script.
-
-    >>> transaction.commit()
+it as a method will recalculate the heat for all the out-of-date bugs.
+
+There are two bugs with heat more than a day old:
 
     >>> getUtility(IBugSet).getBugsWithOutdatedHeat(1).count()
     2
 
-We need to run update_bug_heat() twice to ensure that both the bugs are
-updated.
-
-    >>> update_bug_heat(chunk_size=2)
-    DEBUG Adding CalculateBugHeatJobs for 2 Bugs (starting id: ...)
-    DEBUG Adding CalculateBugHeatJob for bug ...
-    DEBUG Adding CalculateBugHeatJob for bug ...
-
-There will now be two CalculateBugHeatJobs in the queue.
-
-    >>> ready_jobs = list(getUtility(ICalculateBugHeatJobSource).iterReady())
-    >>> len(ready_jobs)
-    2
-
-Running them will update the bugs' heat.
-
-    >>> for calc_job in getUtility(ICalculateBugHeatJobSource).iterReady():
-    ...     calc_job.job.start()
-    ...     calc_job.run()
-    ...     calc_job.job.complete()
+Calling our BugHeatUpdater will update the heat of those bugs.
+
+    >>> update_bug_heat(chunk_size=1)
+    DEBUG Updating heat for 2 bugs
 
 IBugSet.getBugsWithOutdatedHeat() will now return an empty set since all
 the bugs have been updated.
 
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-05-29 16:21:26 +0000
+++ lib/lp/bugs/model/bug.py	2010-05-29 16:21:27 +0000
@@ -73,7 +73,6 @@
 from lp.bugs.interfaces.bugactivity import IBugActivitySet
 from lp.bugs.interfaces.bugattachment import (
     BugAttachmentType, IBugAttachmentSet)
-from lp.bugs.interfaces.bugjob import ICalculateBugHeatJobSource
 from lp.bugs.interfaces.bugmessage import IBugMessageSet
 from lp.bugs.interfaces.bugnomination import (
     NominationError, NominationSeriesObsoleteError)
@@ -490,8 +489,6 @@
         sub = BugSubscription(
             bug=self, person=person, subscribed_by=subscribed_by)
 
-        getUtility(ICalculateBugHeatJobSource).create(self)
-
         # Ensure that the subscription has been flushed.
         Store.of(sub).flush()
 
@@ -501,6 +498,7 @@
         if suppress_notify is False:
             notify(ObjectCreatedEvent(sub, user=subscribed_by))
 
+        self.updateHeat()
         return sub
 
     def unsubscribe(self, person, unsubscribed_by):
@@ -525,6 +523,7 @@
                 # flushed so that code running with implicit flushes
                 # disabled see the change.
                 store.flush()
+                self.updateHeat()
                 return
 
     def unsubscribeFromDupes(self, person, unsubscribed_by):
@@ -803,7 +802,7 @@
                 notification_data['text'], change.person, recipients,
                 when)
 
-        getUtility(ICalculateBugHeatJobSource).create(self)
+        self.updateHeat()
 
     def expireNotifications(self):
         """See `IBug`."""
@@ -1459,7 +1458,7 @@
             if dupe._getAffectedUser(user) is not None:
                 dupe.markUserAffected(user, affected)
 
-        getUtility(ICalculateBugHeatJobSource).create(self)
+        self.updateHeat()
 
     @property
     def readonly_duplicateof(self):
@@ -1470,6 +1469,7 @@
         """See `IBug`."""
         field = DuplicateBug()
         field.context = self
+        current_duplicateof = self.duplicateof
         try:
             if duplicate_of is not None:
                 field._validate(duplicate_of)
@@ -1478,15 +1478,17 @@
             raise InvalidDuplicateValue(validation_error)
 
         if duplicate_of is not None:
-            # Create a job to update the heat of the master bug and set
-            # this bug's heat to 0 (since it's a duplicate, it shouldn't
-            # have any heat at all).
-            getUtility(ICalculateBugHeatJobSource).create(duplicate_of)
+            # Update the heat of the master bug and set this bug's heat
+            # to 0 (since it's a duplicate, it shouldn't have any heat
+            # at all).
+            self.setHeat(0)
+            duplicate_of.updateHeat()
+        else:
+            # Otherwise, recalculate this bug's heat, since it will be 0
+            # from having been a duplicate. We also update the bug that
+            # was previously duplicated.
             self.updateHeat()
-        else:
-            # Otherwise, create a job to recalculate this bug's heat,
-            # since it will be 0 from having been a duplicate.
-            getUtility(ICalculateBugHeatJobSource).create(self)
+            current_duplicateof.updateHeat()
 
     def setCommentVisibility(self, user, comment_number, visible):
         """See `IBug`."""
@@ -1863,7 +1865,8 @@
             Bug.heat_last_updated < last_updated_cutoff,
             Bug.heat_last_updated == None)
 
-        return store.find(Bug, last_updated_clause).order_by('id')
+        return store.find(
+            Bug, Bug.duplicateof==None, last_updated_clause).order_by('id')
 
 
 class BugAffectsPerson(SQLBase):
 
=== modified file 'lib/lp/bugs/tests/test_bugheat.py'
--- lib/lp/bugs/tests/test_bugheat.py	2010-04-01 03:09:43 +0000
+++ lib/lp/bugs/tests/test_bugheat.py	2010-05-29 16:21:27 +0000
@@ -112,108 +112,6 @@
         self.assertIn(('bug_job_id', job.context.id), vars)
         self.assertIn(('bug_job_type', job.context.job_type.title), vars)
 
-    def test_bug_changes_adds_job(self):
-        # Calling addChange() on a Bug will add a CalculateBugHeatJob
-        # for that bug to the queue.
-        self.assertEqual(0, self._getJobCount())
-
-        change = BugDescriptionChange(
-            when=datetime.now().replace(tzinfo=pytz.timezone('UTC')),
-            person=self.bug.owner, what_changed='description',
-            old_value=self.bug.description, new_value='Some text')
-        self.bug.addChange(change)
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_subscribing_adds_job(self):
-        # Calling Bug.subscribe() will add a CalculateBugHeatJob for the
-        # Bug.
-        self.assertEqual(0, self._getJobCount())
-
-        person = self.factory.makePerson()
-        self.bug.subscribe(person, person)
-        transaction.commit()
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_unsubscribing_adds_job(self):
-        # Calling Bug.unsubscribe() will add a CalculateBugHeatJob for the
-        # Bug.
-        self.assertEqual(0, self._getJobCount())
-
-        self.bug.unsubscribe(self.bug.owner, self.bug.owner)
-        transaction.commit()
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_marking_affected_adds_job(self):
-        # Marking a user as affected by a bug adds a CalculateBugHeatJob
-        # for the bug.
-        self.assertEqual(0, self._getJobCount())
-
-        person = self.factory.makePerson()
-        self.bug.markUserAffected(person)
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_marking_unaffected_adds_job(self):
-        # Marking a user as unaffected by a bug adds a CalculateBugHeatJob
-        # for the bug.
-        self.assertEqual(0, self._getJobCount())
-
-        self.bug.markUserAffected(self.bug.owner, False)
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_bug_creation_creates_job(self):
-        # Creating a bug adds a CalculateBugHeatJob for the new bug.
-        self.assertEqual(0, self._getJobCount())
-
-        new_bug = self.factory.makeBug()
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-    def test_marking_dupe_creates_job(self):
-        # Marking a bug as a duplicate of another bug creates a job to
-        # update the master bug.
-        new_bug = self.factory.makeBug()
-        new_bug.setHeat(42)
-        self._completeJobsAndAssertQueueEmpty()
-
-        new_bug.markAsDuplicate(self.bug)
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-        # And the job will be for the master bug.
-        bug_job = self._getJobs()[0]
-        self.assertEqual(bug_job.bug, self.bug)
-
-        # Also, the duplicate bug's heat will have been set to zero.
-        self.assertEqual(0, new_bug.heat)
-
-    def test_unmarking_dupe_creates_job(self):
-        # Unmarking a bug as a duplicate will create a
-        # CalculateBugHeatJob for the bug, since its heat will be 0 from
-        # having been marked as a duplicate.
-        new_bug = self.factory.makeBug()
-        new_bug.markAsDuplicate(self.bug)
-        self._completeJobsAndAssertQueueEmpty()
-        new_bug.markAsDuplicate(None)
-
-        # There will now be a job in the queue.
-        self.assertEqual(1, self._getJobCount())
-
-        # And the job will be for the master bug.
-        bug_job = self._getJobs()[0]
-        self.assertEqual(bug_job.bug, new_bug)
-
 
 class MaxHeatByTargetBase:
     """Base class for testing a bug target's max_bug_heat attribute."""