launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/improve-latestpersonspr-garbo-job-1071581 into lp:launchpad.

Commit message:
Improve PopulateLatestPersonSourcepackageReleaseCache so it only processes new spr records.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1071581 in Launchpad itself: "+ppa-packages timeout"
  https://bugs.launchpad.net/launchpad/+bug/1071581

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/improve-latestpersonspr-garbo-job-1071581/+merge/133417

== Implementation ==

When I first wrote the garbo job, I didn't now that a new source package release record was created each time a package is uploaded. So the garbo job reset the next_id to 0 after each run, causing all spr's to eventually be reprocessed. This branch alters the job so that a separate, monotonically increasing next id is used for processing creator and maintainer records. Thus, the garbo job should eventually "catch" up to the releases and from then on do minimal work to maintain the report table.

== Tests ==

Update the test_PopulateLatestPersonSourcepackageReleaseCache to chec the job state.

== 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/~wallyworld/launchpad/improve-latestpersonspr-garbo-job-1071581/+merge/133417
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/improve-latestpersonspr-garbo-job-1071581 into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-11-05 11:32:27 +0000
+++ lib/lp/scripts/garbo.py	2012-11-08 07:54:22 +0000
@@ -478,13 +478,15 @@
         self.store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
         # Keep a record of the processed source package release id and data
         # type (creator or maintainer) so we know where to job got up to.
-        self.next_id = 0
+        self.next_id_for_creator = 0
+        self.next_id_for_maintainer = 0
         self.current_person_filter_type = 'creator'
         self.starting_person_filter_type = self.current_person_filter_type
         self.job_name = self.__class__.__name__
         job_data = load_garbo_job_state(self.job_name)
         if job_data:
-            self.next_id = job_data['next_id']
+            self.next_id_for_creator = job_data['next_id_for_creator']
+            self.next_id_for_maintainer = job_data['next_id_for_maintainer']
             self.current_person_filter_type = job_data['person_filter_type']
             self.starting_person_filter_type = self.current_person_filter_type
 
@@ -493,8 +495,10 @@
         # creator or maintainer as required.
         if self.current_person_filter_type == 'creator':
             person_filter = SourcePackageRelease.creatorID
+            next_id = self.next_id_for_creator
         else:
             person_filter = SourcePackageRelease.maintainerID
+            next_id = self.next_id_for_maintainer
         spph = ClassAlias(SourcePackagePublishingHistory, "spph")
         origin = [
             SourcePackageRelease,
@@ -504,7 +508,7 @@
                     spph.archiveID == SourcePackageRelease.upload_archiveID))]
         spr_select = self.store.using(*origin).find(
             (SourcePackageRelease.id, Alias(spph.id, 'spph_id')),
-            SourcePackageRelease.id > self.next_id
+            SourcePackageRelease.id > next_id
         ).order_by(
             person_filter,
             SourcePackageRelease.upload_distroseriesID,
@@ -546,7 +550,6 @@
                 self.current_person_filter_type = 'maintainer'
             else:
                 self.current_person_filter_type = 'creator'
-            self.next_id = 0
             current_count = self.getPendingUpdates().count()
         return current_count == 0
 
@@ -601,17 +604,22 @@
             self.store.execute(Insert(columns, values=inserts))
 
     def __call__(self, chunk_size):
-        max_id = self.next_id
+        max_id = None
         updates = []
         for update in (self.getPendingUpdates()[:chunk_size]):
             updates.append(update)
             max_id = update[0]
         self.update_cache(updates)
 
-        self.next_id = max_id
+        if max_id:
+            if self.current_person_filter_type == 'creator':
+                self.next_id_for_creator = max_id
+            else:
+                self.next_id_for_maintainer = max_id
         self.store.flush()
         save_garbo_job_state(self.job_name, {
-            'next_id': max_id,
+            'next_id_for_creator': self.next_id_for_creator,
+            'next_id_for_maintainer': self.next_id_for_maintainer,
             'person_filter_type': self.current_person_filter_type})
         transaction.commit()
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-11-07 12:38:19 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-11-08 07:54:22 +0000
@@ -1205,6 +1205,11 @@
         _assert_release_by_maintainer(maintainers[0], spr3)
         _assert_release_by_maintainer(maintainers[1], spr4)
 
+        job_data = load_garbo_job_state(
+            'PopulateLatestPersonSourcepackageReleaseCache')
+        self.assertEqual(spr4.id, job_data['next_id_for_creator'])
+        self.assertEqual(spr4.id, job_data['next_id_for_maintainer'])
+
         # Create a newer published source package release and ensure the
         # release cache table is correctly updated.
         switch_dbuser('testadmin')
@@ -1224,6 +1229,11 @@
         _assert_release_by_maintainer(maintainers[0], spr3)
         _assert_release_by_maintainer(maintainers[1], spr5)
 
+        job_data = load_garbo_job_state(
+            'PopulateLatestPersonSourcepackageReleaseCache')
+        self.assertEqual(spr5.id, job_data['next_id_for_creator'])
+        self.assertEqual(spr5.id, job_data['next_id_for_maintainer'])
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer