launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/populate-transitively_private-garbo-job into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/populate-transitively_private-garbo-job/+merge/75661

This mp adds a garbo job to populate the new transitively_private column in the branch table. This branch will be landed after the db-devel branches to add the column (and index) and the triggers to maintain the column are deployed. Once all the transitively_private values are populated, this garbo job can be removed.

== Implementation ==

lifeless helped tune the query using his awesone postgres explain skills. So we are happy that it performs adequately.

== Tests ==

Add a new TestGarbo test:
  - test_populate_transitively_private

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  database/schema/security.cfg
  lib/lp/code/model/branch.py
  lib/lp/scripts/garbo.py
  lib/lp/scripts/tests/test_garbo.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/populate-transitively_private-garbo-job/+merge/75661
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/populate-transitively_private-garbo-job into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-09-13 04:31:49 +0000
+++ database/schema/security.cfg	2011-09-16 02:41:24 +0000
@@ -2150,6 +2150,7 @@
 [garbo]
 groups=script,read
 public.answercontact                    = SELECT, DELETE
+public.branch                           = SELECT, UPDATE
 public.branchjob                        = SELECT, DELETE
 public.binarypackagename                = SELECT
 public.binarypackagerelease             = SELECT

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-09-12 07:29:31 +0000
+++ lib/lp/code/model/branch.py	2011-09-16 02:41:24 +0000
@@ -175,6 +175,10 @@
     # of the state of any stacked on branches.
     explicitly_private = BoolCol(
         default=False, notNull=True, dbName='private')
+    # A branch is transitively private if it is private or it is stacked on a
+    # transitively private branch. The value of this attribute is maintained
+    # by a database trigger.
+    transitively_private = BoolCol(dbName='transitively_private')
 
     @property
     def private(self):

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-09-14 13:26:42 +0000
+++ lib/lp/scripts/garbo.py	2011-09-16 02:41:24 +0000
@@ -69,6 +69,7 @@
     MAX_SAMPLE_SIZE,
     )
 from lp.code.interfaces.revision import IRevisionSet
+from lp.code.model.branch import Branch
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportresult import CodeImportResult
 from lp.code.model.revision import (
@@ -747,6 +748,67 @@
         """
 
 
+class PopulateBranchTransitivelyPrivate(TunableLoop):
+    """Populated the branch column transitively_private values.
+
+    Only needed until they are all set, after that triggers will maintain it.
+    """
+
+    maximum_chunk_size = 10000
+
+    def __init__(self, log, abort_time=None):
+        super_instance = super(PopulateBranchTransitivelyPrivate, self)
+        super_instance.__init__(log, abort_time)
+        self.store = IMasterStore(Branch)
+        self.isDone = IMasterStore(Branch).find(
+            Branch, Branch.transitively_private == None).is_empty
+
+    def __call__(self, chunk_size):
+        """See `ITunableLoop`."""
+        transaction.begin()
+        updated = self.store.execute("""
+            UPDATE branch SET transitively_private = stacked_branches.private
+            FROM
+            (
+               WITH recursive stacked_branches AS
+               (
+                  SELECT selected_branch.id,
+                    CASE WHEN transitively_private IS NOT NULL THEN NULL
+                    ELSE selected_branch.stacked_on
+                    END as stacked_on,
+                    COALESCE(transitively_private, private) AS private
+                  FROM branch AS selected_branch
+                  WHERE selected_branch.id IN
+                  (
+                     SELECT id
+                     FROM branch
+                     WHERE transitively_private is NULL
+                     ORDER BY id limit %s
+                  )
+                  UNION all
+                  SELECT
+                    stacked_branches.id,
+                    CASE WHEN branch.transitively_private IS NOT NULL
+                    THEN NULL
+                    ELSE branch.stacked_on
+                    END as stacked_on,
+                    COALESCE(
+                        branch.transitively_private,
+                        branch.private) AS private
+                  FROM stacked_branches JOIN branch
+                  ON stacked_branches.stacked_on = branch.id
+               )
+               SELECT
+               id, bool_or(stacked_branches.private) as private
+               FROM stacked_branches
+               GROUP BY id
+            ) AS stacked_branches WHERE branch.id = stacked_branches.id
+            """ % int(chunk_size)
+            ).rowcount
+        self.log.debug("Updated %s branches." % updated)
+        transaction.commit()
+
+
 class BugHeatUpdater(TunableLoop):
     """A `TunableLoop` for bug heat calculations."""
 
@@ -1259,6 +1321,7 @@
         BugHeatUpdater,
         SourcePackagePublishingHistorySPNPopulator,
         BinaryPackagePublishingHistoryBPNPopulator,
+        PopulateBranchTransitivelyPrivate,
         ]
     experimental_tunable_loops = []
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-09-14 07:04:08 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2011-09-16 02:41:24 +0000
@@ -16,6 +16,7 @@
 
 from pytz import UTC
 from storm.expr import (
+    And,
     In,
     Min,
     Not,
@@ -74,6 +75,7 @@
     )
 from lp.code.enums import CodeImportResultStatus
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
+from lp.code.model.branch import Branch
 from lp.code.model.branchjob import (
     BranchJob,
     BranchUpgradeJob,
@@ -855,6 +857,39 @@
         self._test_AnswerContactPruner(
             AccountStatus.SUSPENDED, ONE_DAY_AGO, expected_count=1)
 
+    def test_populate_transitively_private(self):
+        # Test garbo job to populate the branch transitively_private column.
+
+        # First delete any existing column data and add stacked branches.
+        # Branches 29 and 30 are explicitly private to start with.
+        con = DatabaseLayer._db_fixture.superuser_connection()
+        try:
+            cur = con.cursor()
+            cur.execute("UPDATE branch set stacked_on=29 where id = 5")
+            cur.execute("UPDATE branch set stacked_on=5 where id = 1")
+            cur.execute("UPDATE branch set transitively_private=NULL")
+            con.commit()
+        finally:
+            con.close()
+        store = IMasterStore(Branch)
+        unmigrated = store.find(
+            Branch, Branch.transitively_private == None).count
+        self.assertNotEqual(0, unmigrated())
+        self.runHourly()
+        self.assertEqual(0, unmigrated())
+        # Check the branches that now should be transitively private.
+        self.assertEqual(4, store.find(
+            Branch,
+            And(Branch.transitively_private == True,
+                Branch.id.is_in([1, 5, 29, 30]))
+        ).count())
+        # Check the branches that now should not be transitively private.
+        self.assertEqual(26, store.find(
+            Branch,
+            And(Branch.transitively_private == False,
+                Branch.id < 30)
+        ).count())
+
     def test_BranchJobPruner(self):
         # Garbo should remove jobs completed over 30 days ago.
         LaunchpadZopelessLayer.switchDbUser('testadmin')
@@ -1024,7 +1059,7 @@
         self.assertEqual(0, unreferenced_msgsets.count())
 
     def test_SPPH_and_BPPH_populator(self):
-        # If SPPHs (or BPPHs) do not have sourcepackagename (or 
+        # If SPPHs (or BPPHs) do not have sourcepackagename (or
         # binarypackagename) set, the populator will set it.
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         spph = self.factory.makeSourcePackagePublishingHistory()