launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-623391-transactions into lp:launchpad with lp:~jtv/launchpad/bug-623391 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-623391-transactions/+merge/52168

= Transaction management for DistroSeriesDiff population script =

This builds on my other branch for bug 623391, so you may not find the code that I'm modifying in devel yet.

What I'm adding is transaction management for the script that populates DistroSeriesDiff:
 * Commit after every distroseries, so we can interrupt it and still have progress.
 * Add a --dry-run option that aborts instead.  For performance testing and rehearsals.

One way we could use this script is to run it as part of the next rollout, but interrupt it if takes too long.  After that we can do a manual run, wait for replication etc. to settle down, and repeat.

It's probably not worth building in any special cleverness for that, and we don't yet know how much waiting will be needed.  To give us the flexibility to cover all bases, I added a --list option which just prints out the full list of derived distroseries.  That makes it easy to follow which ones are done and which ones still need doing.

To test,
{{{
./bin/test -vvc lp.registry.scripts.tests
}}}

No lint.


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-623391-transactions/+merge/52168
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-623391-transactions into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/populate_distroseriesdiff.py	2011-03-04 06:19:58 +0000
+++ lib/lp/registry/scripts/populate_distroseriesdiff.py	2011-03-04 06:20:00 +0000
@@ -21,6 +21,7 @@
     OptionValueError,
     )
 from storm.info import ClassAlias
+import transaction
 from zope.component import getUtility
 
 from canonical.database.sqlbase import (
@@ -216,12 +217,15 @@
     return IStore(DistroSeries).find(
         DistroSeries,
         Parent.id == DistroSeries.parent_seriesID,
-        Parent.distributionID != DistroSeries.distributionID)
+        Parent.distributionID != DistroSeries.distributionID).order_by(
+            (DistroSeries.parent_seriesID, DistroSeries.id))
 
 
 class PopulateDistroSeriesDiff(LaunchpadScript):
+    """Populate `DistroSeriesDifference` for pre-existing differences."""
 
     def add_my_options(self):
+        """Register options specific to this script."""
         self.parser.add_options([
             Option(
                 '-a', '--all', dest='all', action='store_true', default=False,
@@ -230,8 +234,14 @@
                 '-d', '--distribution', dest='distribution', default=None,
                 help="Derived distribution."),
             Option(
+                '-l', '--list', dest='list', action='store_true',
+                default=False, help="List derived distroseries, then exit."),
+            Option(
                 '-s', '--series', dest='series', default=None,
-                help="Derived distribution series.")])
+                help="Derived distribution series."),
+            Option(
+                '-x', '--dry-run', dest='dry_run', action='store_true',
+                default=False, help="Pretend; don't commit changes.")])
 
     def getDistroSeries(self):
         """Return the `DistroSeries` that are to be processed."""
@@ -255,8 +265,25 @@
         """Generate `DistroSeriesDifference`s for `distroseries`."""
         self.logger.info("Looking for differences in %s.", distroseries)
         populate_distroseriesdiff(distroseries)
-
-    def main(self):
+        self.commit()
+        self.logger.info("Done with %s.", distroseries)
+
+    def commit(self):
+        """Commit (or if doing a dry run, abort instead)."""
+        if self.options.dry_run:
+            transaction.abort()
+        else:
+            transaction.commit()
+
+    def listDerivedSeries(self):
+        """Log all `DistroSeries` that the --all option would cover."""
+        for series in self.getDistroSeries():
+            self.logger.info("%s %s", series.distribution.name, series.name)
+
+    def checkOptions(self):
+        """Verify command-line options."""
+        if self.options.list:
+            return
         specified_distro = (self.options.distribution is not None)
         specified_series = (self.options.series is not None)
         if specified_distro != specified_series:
@@ -266,5 +293,17 @@
             raise OptionValueError(
                 "Either specify a distribution series, or use --all.")
 
+    def main(self):
+        """Do the script's work."""
+        self.checkOptions()
+
+        if self.options.list:
+            self.options.all = True
+            self.listDerivedSeries()
+            return
+
+        if self.options.dry_run:
+            self.logger.info("Dry run requested.  Not committing changes.")
+
         for series in self.getDistroSeries():
             self.processDistroSeries(series)

=== modified file 'lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py	2011-03-04 06:19:58 +0000
+++ lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py	2011-03-04 06:20:00 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from storm.store import Store
+import transaction
 
 from canonical.database.sqlbase import (
     cursor,
@@ -30,7 +31,10 @@
     populate_distroseriesdiff,
     PopulateDistroSeriesDiff,
     )
-from lp.services.log.logger import DevNullLogger
+from lp.services.log.logger import (
+    BufferLogger,
+    DevNullLogger,
+    )
 from lp.soyuz.interfaces.publishing import (
     active_publishing_status,
     inactive_publishing_status,
@@ -468,3 +472,44 @@
         script.main()
         self.assertNotEqual(
             0, self.getDistroSeriesDiff(spph.distroseries).count())
+
+    def test_commits_changes(self):
+        spph = self.makeSPPH(distroseries=self.makeDerivedDistroSeries())
+        script = self.makeScript([
+            '--distribution', spph.distroseries.distribution.name,
+            '--series', spph.distroseries.name,
+            ])
+        script.main()
+        transaction.abort()
+        # The changes are still in the database despite the abort,
+        # because the script already committed them.
+        self.assertNotEqual(
+            0, self.getDistroSeriesDiff(spph.distroseries).count())
+
+    def test_dry_run_goes_through_the_motions(self):
+        spph = self.makeSPPH(distroseries=self.makeDerivedDistroSeries())
+        script = self.makeScript(['--all', '--dry-run'])
+        script.processDistroSeries = FakeMethod
+        script.main()
+        self.assertNotEqual(0, script.processDistroSeries.call_count)
+
+    def test_dry_run_does_not_commit_changes(self):
+        spph = self.makeSPPH(distroseries=self.makeDerivedDistroSeries())
+        transaction.commit()
+        script = self.makeScript([
+            '--distribution', spph.distroseries.distribution.name,
+            '--series', spph.distroseries.name,
+            '--dry-run',
+            ])
+        script.main()
+        self.assertContentEqual(
+            [], self.getDistroSeriesDiff(spph.distroseries))
+
+    def test_list(self):
+        spph = self.makeSPPH(distroseries=self.makeDerivedDistroSeries())
+        script = self.makeScript(['--list'])
+        script.logger = BufferLogger()
+        script.main()
+        expected_series_name = "%s %s" % (
+            spph.distroseries.distribution.name, spph.distroseries.name)
+        self.assertIn(expected_series_name, script.logger.getLogBuffer())