launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-919116 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #919116 in Launchpad itself: "BuilderSet:+index KeyError in preloadSpecificJobData"
  https://bugs.launchpad.net/launchpad/+bug/919116

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-919116/+merge/97587

Bug #919116 has eluded diagnosis for some time, but I finally worked it out today. The BuilderSet:+index preloading code was assumes that Job.id == BuildQueue.id == SourcePackageRecipeBuild.id, which the lack of sampledata in those tables conveniently made true in the test suite. Most of the time this short-circuits because none are found, but occasionally a random row is found and the rest of the preloading blows up. So the preloading either crashes or doesn't do anything at all :)

I fixed the invalid assumptions and adjusted the tests to confirm that the query counts are constant. specific_jobs was also being evaluated a few times, so I changed it to listify once.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-919116/+merge/97587
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-919116 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2012-01-04 16:39:15 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2012-03-15 08:57:26 +0000
@@ -45,6 +45,7 @@
     IBuildQueue,
     IBuildQueueSet,
     )
+from lp.services.database.bulk import load_related
 from lp.services.database.constants import DEFAULT
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.sqlbase import (
@@ -157,12 +158,13 @@
         for job_type, grouped_queues in groupby(queues, key=key):
             specific_class = specific_job_classes()[job_type]
             queue_subset = list(grouped_queues)
+            job_subset = load_related(Job, queue_subset, ['jobID'])
             # We need to preload the build farm jobs early to avoid
             # the call to _set_build_farm_job to look up BuildFarmBuildJobs
             # one by one.
-            specific_class.preloadBuildFarmJobs(queue_subset)
-            specific_jobs = specific_class.getByJobs(queue_subset)
-            if len(list(specific_jobs)) == 0:
+            specific_class.preloadBuildFarmJobs(job_subset)
+            specific_jobs = list(specific_class.getByJobs(job_subset))
+            if len(specific_jobs) == 0:
                 continue
             specific_class.preloadJobsData(specific_jobs)
             specific_jobs_dict = dict(

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2012-01-04 20:12:28 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2012-03-15 08:57:26 +0000
@@ -444,7 +444,7 @@
             )
         return list(IStore(SourcePackageRecipeBuildJob).find(
             SourcePackageRecipeBuild,
-            [SourcePackageRecipeBuildJob.id.is_in([job.id for job in jobs]),
+            [SourcePackageRecipeBuildJob.job_id.is_in([j.id for j in jobs]),
              SourcePackageRecipeBuildJob.build_id ==
                  SourcePackageRecipeBuild.id]))
 

=== modified file 'lib/lp/soyuz/browser/tests/test_builder.py'
--- lib/lp/soyuz/browser/tests/test_builder.py	2012-01-05 14:34:21 +0000
+++ lib/lp/soyuz/browser/tests/test_builder.py	2012-03-15 08:57:26 +0000
@@ -5,12 +5,13 @@
 
 __metaclass__ = type
 
-from testtools.matchers import LessThan
+from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.services.job.model.job import Job
 from lp.services.webapp import canonical_url
 from lp.soyuz.browser.tests.test_builder_views import BuildCreationMixin
 from lp.testing import (
@@ -67,10 +68,11 @@
 
     layer = LaunchpadFunctionalLayer
 
-    # XXX rvb: the query issued per build is the result of the call to
-    # build.buildqueue_record.  It was decided not to make it a cachedproperty
-    # because the code relies on the fact that this property always returns
-    # the current record.
+    def setUp(self):
+        super(TestBuildersHomepage, self).setUp()
+        # Create a non-buildfarm job to ensure that the BuildQueue and
+        # Job IDs differ, detecting bug #919116.
+        Job()
 
     def test_builders_binary_package_build_query_count(self):
         def create_build():
@@ -81,10 +83,7 @@
         nb_objects = 2
         recorder1, recorder2 = record_two_runs(
             builders_homepage_render, create_build, nb_objects)
-
-        self.assertThat(
-            recorder2,
-            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def test_builders_recipe_build_query_count(self):
         def create_build():
@@ -95,10 +94,7 @@
         nb_objects = 2
         recorder1, recorder2 = record_two_runs(
             builders_homepage_render, create_build, nb_objects)
-
-        self.assertThat(
-            recorder2,
-            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def test_builders_translation_template_build_query_count(self):
         def create_build():
@@ -106,16 +102,10 @@
             branch = self.factory.makeBranch()
             specific_job = jobset.create(branch)
             queueset = getUtility(IBuildQueueSet)
-            # Using rSP is required to get the job id.
-            naked_job = removeSecurityProxy(specific_job.job)
-            job_id = naked_job.id
-            queue = queueset.get(job_id)
+            queue = queueset.getByJob(specific_job.job)
             queue.markAsBuilding(self.factory.makeBuilder())
 
         nb_objects = 2
         recorder1, recorder2 = record_two_runs(
             builders_homepage_render, create_build, nb_objects)
-
-        self.assertThat(
-            recorder2,
-            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))