Merge lp:~jtv/launchpad/bug-884649-branch-5 into lp:launchpad

[Jeroen T. Vermeulen]

[r=julian-edwards][bug=884649] Use denormalized [BS]PPH.[bs]pn columns in dominator.

= Summary =

To get to a source package publication record's source package name, we no longer have to go through the associated source package release. There is now a column on SourcePackagePublishingHistory that gives us the SourcePackageName. Similar for BinaryPackagePublishingHistory & BinaryPackageName.

== Pre-implementation notes ==

The work to introduce these new columns wasn't moving along so I helped optimize the populators, kept an eye on them, retired them, added NOT NULL constraints, and initialized the sample data accordingly.

Meanwhile, on the flip side of the coin, domination is actually pretty fast already now. It probably takes less than 10% of the publication process that we're trying to speed up (though it's close enough to its current run-time target that a few percent of optimization are probably still welcome). I chose to keep thebranch because it's also a chance to make things just a bit shorter.

== Implementation details ==

Sadly, the extra column on BPPH doesn't seem to help the dominator all that much — at least not in terms of code simplicity. Most use-cases need the BinaryPackageRelease as well anyway.

== Tests ==

{{{
./bin/test -vvc lp.archvepublisher -t dominator
}}}

== Demo and Q/A ==

Run publish-ftpmaster for Ubuntu. Domination should still work.

Similarly, run gina and verify domination for Debian.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/domination.py
  lib/lp/archivepublisher/tests/test_dominator.py

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2011-12-19 23:38:16 +0000
+++ lib/lp/archivepublisher/domination.py	2011-12-21 09:51:27 +0000
@@ -98,10 +98,15 @@
 apt_pkg.InitSystem()
 
 
-def join_spr_spn():
-    """Join condition: SourcePackageRelease/SourcePackageName."""
-    return (
-        SourcePackageName.id == SourcePackageRelease.sourcepackagenameID)
+def join_spph_spn():
+    """Join condition: SourcePackagePublishingHistory/SourcePackageName."""
+    # Avoid circular imports.
+    from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+
+    SPPH = SourcePackagePublishingHistory
+    SPN = SourcePackageName
+
+    return SPN.id == SPPH.sourcepackagenameID
 
 
 def join_spph_spr():
@@ -110,9 +115,10 @@
     # Avoid circular imports.
     from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
-    return (
-        SourcePackageRelease.id ==
-            SourcePackagePublishingHistory.sourcepackagereleaseID)
+    SPPH = SourcePackagePublishingHistory
+    SPR = SourcePackageRelease
+
+    return SPR.id == SPPH.sourcepackagereleaseID
 
 
 class SourcePublicationTraits:
@@ -127,7 +133,7 @@
     @staticmethod
     def getPackageName(spph):
         """Return the name of this publication's source package."""
-        return spph.sourcepackagerelease.sourcepackagename.name
+        return spph.sourcepackagename.name
 
     @staticmethod
     def getPackageRelease(spph):
@@ -147,7 +153,7 @@
     @staticmethod
     def getPackageName(bpph):
         """Return the name of this publication's binary package."""
-        return bpph.binarypackagerelease.binarypackagename.name
+        return bpph.binarypackagename.name
 
     @staticmethod
     def getPackageRelease(bpph):
@@ -178,12 +184,6 @@
         """Obtain the version string for a publication record."""
         return self.traits.getPackageRelease(pub).version
 
-    def load_releases(self, pubs):
-        """Load the releases associated with a series of publications."""
-        return load_related(
-            self.traits.release_class, pubs,
-            [self.traits.release_reference_name])
-
     def compare(self, pub1, pub2):
         """Compare publications by version.
 
@@ -201,17 +201,7 @@
 
     def sortPublications(self, publications):
         """Sort publications from most to least current versions."""
-        # Listify; we want to iterate this twice, which won't do for a
-        # non-persistent sequence.
-        publications = list(publications)
-        # Batch-load associated package releases; we'll be needing them
-        # to compare versions.
-        self.load_releases(publications)
-        # Now sort.  This is that second iteration.  An in-place sort
-        # won't hurt the original, because we're working on a copy of
-        # the original iterable.
-        publications.sort(cmp=self.compare, reverse=True)
-        return publications
+        return sorted(publications, cmp=self.compare, reverse=True)
 
 
 def find_live_source_versions(sorted_pubs):
@@ -335,12 +325,20 @@
         has active arch-specific publications.
     :return: A list of live versions.
     """
+    # Avoid circular imports
+    from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+
     sorted_pubs = list(sorted_pubs)
     latest = sorted_pubs.pop(0)
     is_arch_specific = attrgetter('architecture_specific')
     arch_specific_pubs = list(ifilter(is_arch_specific, sorted_pubs))
     arch_indep_pubs = list(ifilterfalse(is_arch_specific, sorted_pubs))
 
+    bpbs = load_related(
+        BinaryPackageBuild,
+        [pub.binarypackagerelease for pub in arch_indep_pubs], ['buildID'])
+    load_related(SourcePackageRelease, bpbs, ['source_package_release_id'])
+
     reprieved_pubs = [
         pub
         for pub in arch_indep_pubs
@@ -577,46 +575,33 @@
         # Avoid circular imports.
         from lp.soyuz.model.publishing import BinaryPackagePublishingHistory
 
+        BPPH = BinaryPackagePublishingHistory
+        BPR = BinaryPackageRelease
+
         bpph_location_clauses = [
-            BinaryPackagePublishingHistory.status ==
-                PackagePublishingStatus.PUBLISHED,
-            BinaryPackagePublishingHistory.distroarchseries ==
-                distroarchseries,
-            BinaryPackagePublishingHistory.archive == self.archive,
-            BinaryPackagePublishingHistory.pocket == pocket,
+            BPPH.status == PackagePublishingStatus.PUBLISHED,
+            BPPH.distroarchseries == distroarchseries,
+            BPPH.archive == self.archive,
+            BPPH.pocket == pocket,
             ]
         candidate_binary_names = Select(
-            BinaryPackageName.id,
-            And(
-                BinaryPackageRelease.binarypackagenameID ==
-                    BinaryPackageName.id,
-                BinaryPackagePublishingHistory.binarypackagereleaseID ==
-                    BinaryPackageRelease.id,
-                bpph_location_clauses,
-            ),
-            group_by=BinaryPackageName.id,
-            having=Count(BinaryPackagePublishingHistory.id) > 1)
-        main_clauses = [
-            BinaryPackageRelease.id ==
-                BinaryPackagePublishingHistory.binarypackagereleaseID,
-            BinaryPackageRelease.binarypackagenameID.is_in(
-                candidate_binary_names),
-            BinaryPackageRelease.binpackageformat !=
-                BinaryPackageFormat.DDEB,
+            BPPH.binarypackagenameID, And(*bpph_location_clauses),
+            group_by=BPPH.binarypackagenameID, having=(Count() > 1))
+        main_clauses = bpph_location_clauses + [
+            BPR.id == BPPH.binarypackagereleaseID,
+            BPR.binarypackagenameID.is_in(candidate_binary_names),
+            BPR.binpackageformat != BinaryPackageFormat.DDEB,
             ]
-        main_clauses.extend(bpph_location_clauses)
-
-        store = IStore(BinaryPackagePublishingHistory)
 
         # We're going to access the BPRs as well.  Since we make the
         # database look them up anyway, and since there won't be many
         # duplications among them, load them alongside the publications.
         # We'll also want their BinaryPackageNames, but adding those to
         # the join would complicate the query.
-        query = store.find(
-            (BinaryPackagePublishingHistory, BinaryPackageRelease),
-            *main_clauses)
-        return DecoratedResultSet(query, itemgetter(0))
+        query = IStore(BPPH).find((BPPH, BPR), *main_clauses)
+        bpphs = list(DecoratedResultSet(query, itemgetter(0)))
+        load_related(BinaryPackageName, bpphs, ['binarypackagenameID'])
+        return bpphs
 
     def dominateBinaries(self, distroseries, pocket):
         """Perform domination on binary package publications.
@@ -684,12 +669,13 @@
         # Avoid circular imports.
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
+        SPPH = SourcePackagePublishingHistory
+
         return And(
-            SourcePackagePublishingHistory.status ==
-                PackagePublishingStatus.PUBLISHED,
-            SourcePackagePublishingHistory.distroseries == distroseries,
-            SourcePackagePublishingHistory.archive == self.archive,
-            SourcePackagePublishingHistory.pocket == pocket,
+            SPPH.status == PackagePublishingStatus.PUBLISHED,
+            SPPH.distroseries == distroseries,
+            SPPH.archive == self.archive,
+            SPPH.pocket == pocket,
             )
 
     def findSourcesForDomination(self, distroseries, pocket):
@@ -706,15 +692,16 @@
         # Avoid circular imports.
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
+        SPPH = SourcePackagePublishingHistory
+        SPR = SourcePackageRelease
+
         spph_location_clauses = self._composeActiveSourcePubsCondition(
             distroseries, pocket)
-        having_multiple_active_publications = (
-            Count(SourcePackagePublishingHistory.id) > 1)
         candidate_source_names = Select(
-            SourcePackageName.id,
-            And(join_spph_spr(), join_spr_spn(), spph_location_clauses),
-            group_by=SourcePackageName.id,
-            having=having_multiple_active_publications)
+            SPPH.sourcepackagenameID,
+            And(join_spph_spr(), spph_location_clauses),
+            group_by=SPPH.sourcepackagenameID,
+            having=(Count() > 1))
 
         # We'll also access the SourcePackageReleases associated with
         # the publications we find.  Since they're in the join anyway,
@@ -722,13 +709,14 @@
         # Actually we'll also want the SourcePackageNames, but adding
         # those to the (outer) query would complicate it, and
         # potentially slow it down.
-        query = IStore(SourcePackagePublishingHistory).find(
-            (SourcePackagePublishingHistory, SourcePackageRelease),
+        query = IStore(SPPH).find(
+            (SPPH, SPR),
             join_spph_spr(),
-            SourcePackageRelease.sourcepackagenameID.is_in(
-                candidate_source_names),
+            SPPH.sourcepackagenameID.is_in(candidate_source_names),
             spph_location_clauses)
-        return DecoratedResultSet(query, itemgetter(0))
+        spphs = DecoratedResultSet(query, itemgetter(0))
+        load_related(SourcePackageName, spphs, ['sourcepackagenameID'])
+        return spphs
 
     def dominateSources(self, distroseries, pocket):
         """Perform domination on source package publications.
@@ -771,7 +759,7 @@
         result = IStore(SourcePackageName).find(
             looking_for,
             join_spph_spr(),
-            join_spr_spn(),
+            join_spph_spn(),
             self._composeActiveSourcePubsCondition(distroseries, pocket))
         return result.group_by(SourcePackageName.name)
 
@@ -780,18 +768,19 @@
         # Avoid circular imports.
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 
+        SPPH = SourcePackagePublishingHistory
+        SPR = SourcePackageRelease
+
         query = IStore(SourcePackagePublishingHistory).find(
-            SourcePackagePublishingHistory,
+            SPPH,
             join_spph_spr(),
-            join_spr_spn(),
+            join_spph_spn(),
             SourcePackageName.name == package_name,
             self._composeActiveSourcePubsCondition(distroseries, pocket))
         # Sort by descending version (SPR.version has type debversion in
         # the database, so this should be a real proper comparison) so
         # that _sortPackage will have slightly less work to do later.
-        return query.order_by(
-            Desc(SourcePackageRelease.version),
-            Desc(SourcePackagePublishingHistory.datecreated))
+        return query.order_by(Desc(SPR.version), Desc(SPPH.datecreated))
 
     def dominateSourceVersions(self, distroseries, pocket, package_name,
                                live_versions):
 
=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2011-11-04 19:11:18 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2011-12-21 09:51:27 +0000
@@ -473,19 +473,6 @@
             bpph.binarypackagerelease.version,
             GeneralizedPublication(is_source=False).getPackageVersion(bpph))
 
-    def test_load_releases_loads_sourcepackagerelease(self):
-        spph = self.factory.makeSourcePackagePublishingHistory()
-        self.assertContentEqual(
-            [spph.sourcepackagerelease],
-            GeneralizedPublication(is_source=True).load_releases([spph]))
-
-    def test_load_releases_loads_binarypackagerelease(self):
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=self.factory.makeBinaryPackageRelease())
-        self.assertContentEqual(
-            [bpph.binarypackagerelease],
-            GeneralizedPublication(is_source=False).load_releases([bpph]))
-
     def test_compare_sorts_versions(self):
         versions = [
             '1.1v2',