launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-849683-populators into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #849683 in Launchpad itself: "{B,S}PPH Populator are temporary"
  https://bugs.launchpad.net/launchpad/+bug/849683

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-849683-populators/+merge/85461

= Summary =

Two new denormalized database columns have now been fully populated:

    SourcePackagePublishingHistory.sourcepackagename
    BinaryPackagePublishingHistory.binarypackagename

Database constraints (“NOT NULL”) to enforce this are on the way in.  No nulls have appeared in these columns over the past few weeks.

With that out of the way, we can retire the populators from garbo-hourly.  Which is what this branch does.

An earlier attempt to do this was nixed on the basis that the populators might conceivably still be needed before the new constraints were deployed, to deal with any remaining initialization bugs that had yet to manifest themselves.  In that event, still having the populators active might possibly increase the chances that the database branch would be deployable despite being incompatible with the running application, saving us holdups in the deployment pipeline; and still having them in the codebase would also have saved us the trouble of looking up and restoring the populator code from bzr history in the event that a sudden burst of incorrect data would have been too large to clean up with direct SQL.  The certain cost of this caution would be a bit more obsolete cruft to clean up, a bit more time and focus needed to complete this work, and a bit more time spent on unnecessary garbo jobs.

A few weeks' hindsight since then tells us that this was all moot; we now know that we could have deployed safely and moved ahead, as expected at the time, and have this finished by now.  Things may still break, but if they do, the most likely cause is the delay introduced by our abundance of caution.


== Tests ==

{{{
./bin/test -vvc lp.scripts.tests.test_garbo
}}}


== Demo and Q/A ==

Not really the same thing, but keep an eye on anythng that might indicate that nulls are being written to these columns.  The most likely outcome would be jobs failing with constraint violations.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/scripts/garbo.py
  lib/lp/scripts/tests/test_garbo.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-849683-populators/+merge/85461
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-849683-populators into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-12-09 02:19:10 +0000
+++ lib/lp/scripts/garbo.py	2011-12-13 10:52:29 +0000
@@ -85,10 +85,6 @@
     SilentLaunchpadScriptFailure,
     )
 from lp.services.session.model import SessionData
-from lp.soyuz.model.publishing import (
-    BinaryPackagePublishingHistory,
-    SourcePackagePublishingHistory,
-    )
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.model.potmsgset import POTMsgSet
 from lp.translations.model.potranslation import POTranslation
@@ -965,86 +961,6 @@
         transaction.commit()
 
 
-# XXX: StevenK 2011-09-14 bug=849683: This can be removed when done.
-class SourcePackagePublishingHistorySPNPopulator(TunableLoop):
-    """Populate the new sourcepackagename column of SPPH."""
-
-    done = False
-    maximum_chunk_size = 5000
-
-    SPPH = SourcePackagePublishingHistory
-
-    def getStore(self):
-        return IMasterStore(self.SPPH)
-
-    def findSPPHs(self):
-        SPPH = self.SPPH
-        return self.getStore().find(SPPH.id, SPPH.sourcepackagename == None)
-
-    def isDone(self):
-        """See `TunableLoop`."""
-        return self.done
-
-    def __call__(self, chunk_size):
-        """See `TunableLoop`."""
-        spphs = list(self.findSPPHs()[:chunk_size])
-        self.log.info("Populating %d SPPH(s).", len(spphs))
-        if len(spphs) == 0:
-            self.log.info("Finished populating SPPHs.  Remove the populator.")
-            self.done = True
-            return
-        self.getStore().execute("""
-            UPDATE SourcePackagePublishingHistory AS SPPH
-            SET sourcepackagename = SPR.sourcepackagename
-            FROM SourcePackageRelease AS SPR
-            WHERE
-                SPR.id = SPPH.sourcepackagerelease AND
-                SPPH.sourcepackagename IS NULL AND
-                SPPH.id IN %s
-            """ % sqlvalues(spphs))
-        transaction.commit()
-
-
-# XXX: StevenK 2011-09-14 bug=849683: This can be removed when done.
-class BinaryPackagePublishingHistoryBPNPopulator(TunableLoop):
-    """Populate the new binarypackagename column of BPPH."""
-
-    done = False
-    maximum_chunk_size = 5000
-
-    BPPH = BinaryPackagePublishingHistory
-
-    def getStore(self):
-        return IMasterStore(self.BPPH)
-
-    def findBPPHs(self):
-        BPPH = self.BPPH
-        return self.getStore().find(BPPH.id, BPPH.binarypackagename == None)
-
-    def isDone(self):
-        """See `TunableLoop`."""
-        return self.done
-
-    def __call__(self, chunk_size):
-        """See `TunableLoop`."""
-        bpphs = list(self.findBPPHs()[:chunk_size])
-        self.log.info("Populating %d BPPH(s).", len(bpphs))
-        if len(bpphs) == 0:
-            self.log.info("Finished populating BPPHs.  Remove the populator.")
-            self.done = True
-            return
-        self.getStore().execute("""
-            UPDATE BinaryPackagePublishingHistory AS BPPH
-            SET binarypackagename = BPR.binarypackagename
-            FROM BinaryPackageRelease AS BPR
-            WHERE
-                BPR.id = BPPH.binarypackagerelease AND
-                BPPH.binarypackagename IS NULL AND
-                BPPH.id IN %s
-            """ % sqlvalues(bpphs))
-        transaction.commit()
-
-
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -1294,8 +1210,6 @@
         UnusedSessionPruner,
         DuplicateSessionPruner,
         BugHeatUpdater,
-        SourcePackagePublishingHistorySPNPopulator,
-        BinaryPackagePublishingHistoryBPNPopulator,
         ]
     experimental_tunable_loops = []
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-12-09 02:19:10 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2011-12-13 10:52:29 +0000
@@ -1026,23 +1026,6 @@
         self.runDaily()
         self.assertEqual(0, unreferenced_msgsets.count())
 
-    def test_SPPH_and_BPPH_populator(self):
-        # If SPPHs (or BPPHs) do not have sourcepackagename (or
-        # binarypackagename) set, the populator will set it.
-        LaunchpadZopelessLayer.switchDbUser('testadmin')
-        spph = self.factory.makeSourcePackagePublishingHistory()
-        spn = spph.sourcepackagename
-        removeSecurityProxy(spph).sourcepackagename = None
-        bpph = self.factory.makeBinaryPackagePublishingHistory()
-        bpn = bpph.binarypackagename
-        removeSecurityProxy(bpph).binarypackagename = None
-        transaction.commit()
-        self.assertIs(None, spph.sourcepackagename)
-        self.assertIs(None, bpph.binarypackagename)
-        self.runHourly()
-        self.assertEqual(spn, spph.sourcepackagename)
-        self.assertEqual(bpn, bpph.binarypackagename)
-
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer