launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/flag-bugtaskflat-garbo into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #989209 in Launchpad itself: "Timeout searching ubuntu patches"
  https://bugs.launchpad.net/launchpad/+bug/989209

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/flag-bugtaskflat-garbo/+merge/104875

Teach BugTaskFlattener to generate its watermark memcached key from a feature-flagged generation value. This lets us disable the flattener when we don't need it (most of the time) and easily trigger a full regeneration when we make a schema change.
-- 
https://code.launchpad.net/~wgrant/launchpad/flag-bugtaskflat-garbo/+merge/104875
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/flag-bugtaskflat-garbo into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-05-07 00:21:39 +0000
+++ lib/lp/scripts/garbo.py	2012-05-07 07:45:23 +0000
@@ -1128,11 +1128,19 @@
 
     def __init__(self, log, abort_time=None):
         super(BugTaskFlattener, self).__init__(log, abort_time)
-        watermark = getUtility(IMemcacheClient).get(
-            '%s:bugtask-flattener' % config.instance_name)
-        self.start_at = watermark or 0
+        generation = getFeatureFlag('bugs.bugtaskflattener.generation')
+        if generation is None:
+            self.start_at = None
+        else:
+            self.memcache_key = (
+                '%s:bugtask-flattener:%s'
+                % (config.instance_name, generation.encode('utf-8')))
+            watermark = getUtility(IMemcacheClient).get(self.memcache_key)
+            self.start_at = watermark or 0
 
     def findTaskIDs(self):
+        if self.start_at is None:
+            return EmptyResultSet()
         return IMasterStore(BugTask).find(
             (BugTask.id,), BugTask.id >= self.start_at).order_by(BugTask.id)
 
@@ -1147,8 +1155,7 @@
 
         self.start_at = ids[-1] + 1
         result = getUtility(IMemcacheClient).set(
-            '%s:bugtask-flattener' % config.instance_name,
-            self.start_at)
+            self.memcache_key, self.start_at)
         if not result:
             self.log.warning('Failed to set start_at in memcache.')
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-05-04 06:24:08 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-05-07 07:45:23 +0000
@@ -109,7 +109,10 @@
     TestCase,
     TestCaseWithFactory,
     )
-from lp.testing.dbuser import switch_dbuser
+from lp.testing.dbuser import (
+    dbuser,
+    switch_dbuser,
+    )
 from lp.testing.layers import (
     DatabaseLayer,
     LaunchpadScriptLayer,
@@ -1107,33 +1110,45 @@
         self.assertEqual(0, spec.work_items.count())
 
     def test_BugTaskFlattener(self):
-        # Private bugs without corresponding data in the access policy
-        # schema get mirrored.
-        switch_dbuser('testadmin')
-        task = self.factory.makeBugTask()
+        # Bugs without a record in BugTaskFlat get mirrored.
         # Remove the existing mirrored data.
-        IMasterStore(BugTask).execute(
-            'DELETE FROM BugTaskFlat WHERE bugtask = ?', (task.id,))
-        transaction.commit()
-        self.runHourly()
-        # Check that there's a record again, and delete it.
-        switch_dbuser('testadmin')
-        self.assertEqual(
-            (task.id,),
+        with dbuser('testadmin'):
+            task = self.factory.makeBugTask()
             IMasterStore(BugTask).execute(
+                'DELETE FROM BugTaskFlat WHERE bugtask = ?', (task.id,))
+
+        def get_flat():
+            return IMasterStore(BugTask).execute(
                 'SELECT bugtask FROM BugTaskFlat WHERE bugtask = ?',
-                (task.id,)).get_one())
-        IMasterStore(BugTask).execute(
-            'DELETE FROM BugTaskFlat WHERE bugtask = ?', (task.id,))
-        transaction.commit()
-        self.runHourly()
+                (task.id,)).get_one()
+
+        # Nothing is done until the feature flag is set.
+        self.runHourly()
+        self.assertIs(None, get_flat())
+
+        # If we set the generation flag, the bug will be mirrored.
+        with dbuser('testadmin'):
+            IMasterStore(FeatureFlag).add(FeatureFlag(
+                u'default', 0, u'bugs.bugtaskflattener.generation', u'1'))
+        self.runHourly()
+        self.assertEqual((task.id,), get_flat())
+
         # A watermark is kept in memcache, so a second run doesn't
         # consider the same task.
-        self.assertIs(
-            None,
+        with dbuser('testadmin'):
             IMasterStore(BugTask).execute(
-                'SELECT bugtask FROM BugTaskFlat WHERE bugtask = ?',
-                (task.id,)).get_one())
+                'DELETE FROM BugTaskFlat WHERE bugtask = ?', (task.id,))
+        self.runHourly()
+        self.assertIs(None, get_flat())
+
+        # Incrementing the generation feature flag causes a fresh pass.
+        with dbuser('testadmin'):
+            IMasterStore(FeatureFlag).find(
+                FeatureFlag, flag=u'bugs.bugtaskflattener.generation').remove()
+            IMasterStore(FeatureFlag).add(FeatureFlag(
+                u'default', 1, u'bugs.bugtaskflattener.generation', u'2'))
+        self.runHourly()
+        self.assertEqual((task.id,), get_flat())
 
     def test_BranchInformationTypeMigrator_public(self):
         # A non-migrated public branch will have information_type set

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-05-01 07:31:48 +0000
+++ lib/lp/services/features/flags.py	2012-05-07 07:45:23 +0000
@@ -63,6 +63,14 @@
      '',
      '',
      'https://bugs.launchpad.net/launchpad/+bug/678090'),
+    ('bugs.bugtaskflattener.generation',
+     'string',
+     ("Sets the key used to store progress in a BugTaskFlat update pass. "
+      "Normally disabled unless recent schema changes require a full "
+      "update."),
+     'BugTaskFlattener disabled',
+     '',
+     ''),
     ('bugs.bugtracker_components.enabled',
      'boolean',
      ('Enables the display of bugtracker components.'),