Merge lp:~rvb/launchpad/builders-timeout-903827-2 into lp:launchpad

[Raphaël Badin]

[r=jcsackett][bug=903827] Make buildqueue.specific_job and builder.currentjob cached properties.

This branch makes buildqueue.specific_job and builder.currentjob cached properties. builder.currentjob is eager loaded in lib/lp/soyuz/browser/builder and buildqueue.specific_job (which should not change over time) is loaded in preloadSpecificJobData.

= Tests =
(modified)
test_builder test_builders_binary_package_build_query_count
test_builder test_builders_recipe_build_query_count
test_builder test_builders_translation_template_build_query_count

= Q/A =
The number of repeated statements of https://launchpad.net/builders should diminish a great deal. It's currently around in [300 - 600] depending on the type of builds building. Note that the repeated statement issued to fetch buildqueue records will still be there.

[j.c.sackett (jcsackett)]

This looks like a good improvement.

review: Approve

=== modified file 'lib/lp/buildmaster/doc/buildqueue.txt'
--- lib/lp/buildmaster/doc/buildqueue.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/buildmaster/doc/buildqueue.txt	2012-01-06 15:32:25 +0000
@@ -13,6 +13,7 @@
 collected) the BuildQueue record representing it is removed.
 
     >>> from lp.services.webapp.testing import verifyObject
+    >>> from lp.services.propertycache import get_property_cache
     >>> from lp.buildmaster.interfaces.buildqueue import (
     ...     IBuildQueue, IBuildQueueSet)
 
@@ -135,6 +136,7 @@
 
     >>> job.reset()
 
+    >>> del get_property_cache(bob).currentjob
     >>> print bob.currentjob
     None
 
@@ -154,6 +156,7 @@
 
     >>> job.markAsBuilding(bob)
 
+    >>> del get_property_cache(bob).currentjob
     >>> bob.currentjob == job
     True
 

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2012-01-03 12:17:29 +0000
+++ lib/lp/buildmaster/manager.py	2012-01-06 15:32:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Soyuz buildd slave manager logic."""
@@ -34,6 +34,7 @@
     BuildBehaviorMismatch,
     )
 from lp.buildmaster.model.builder import Builder
+from lp.services.propertycache import get_property_cache
 from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 
 
@@ -50,7 +51,9 @@
 def assessFailureCounts(builder, fail_notes):
     """View builder/job failure_count and work out which needs to die.  """
     # builder.currentjob hides a complicated query, don't run it twice.
-    # See bug 623281.
+    # See bug 623281 (Note that currentjob is a cachedproperty).
+
+    del get_property_cache(builder).currentjob
     current_job = builder.currentjob
     if current_job is None:
         job_failure_count = 0
@@ -64,6 +67,7 @@
         # we can do is try them both again, and hope that the job
         # runs against a different builder.
         current_job.reset()
+        del get_property_cache(builder).currentjob
         return
 
     if builder.failure_count > job_failure_count:
@@ -96,6 +100,7 @@
         # but that would cause us to query the slave for its status
         # again, and if the slave is non-responsive it holds up the
         # next buildd scan.
+    del get_property_cache(builder).currentjob
 
 
 class SlaveScanner:

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2012-01-03 12:11:57 +0000
+++ lib/lp/buildmaster/model/builder.py	2012-01-06 15:32:25 +0000
@@ -431,6 +431,7 @@
 
     def _getCurrentBuildBehavior(self):
         """Return the current build behavior."""
+        self._clean_currentjob_cache()
         if not safe_hasattr(self, '_current_build_behavior'):
             self._current_build_behavior = None
 
@@ -479,10 +480,12 @@
     def gotFailure(self):
         """See `IBuilder`."""
         self.failure_count += 1
+        self._clean_currentjob_cache()
 
     def resetFailureCount(self):
         """See `IBuilder`."""
         self.failure_count = 0
+        self._clean_currentjob_cache()
 
     def rescueIfLost(self, logger=None):
         """See `IBuilder`."""
@@ -498,11 +501,14 @@
 
     # XXX 2010-08-24 Julian bug=623281
     # This should not be a property!  It's masking a complicated query.
-    @property
+    @cachedproperty
     def currentjob(self):
         """See IBuilder"""
         return getUtility(IBuildQueueSet).getByBuilder(self)
 
+    def _clean_currentjob_cache(self):
+        del get_property_cache(self).currentjob
+
     def requestAbort(self):
         """See IBuilder."""
         return self.slave.abort()

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2012-01-02 11:21:14 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2012-01-06 15:32:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -53,6 +53,10 @@
     )
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.services.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
@@ -138,12 +142,15 @@
         """See `IBuildQueue`."""
         return IBuildFarmJobBehavior(self.specific_job)
 
-    @property
+    @cachedproperty
     def specific_job(self):
         """See `IBuildQueue`."""
         specific_class = specific_job_classes()[self.job_type]
         return specific_class.getByJob(self.job)
 
+    def _clear_specific_job_cache(self):
+        del get_property_cache(self).specific_job
+
     @staticmethod
     def preloadSpecificJobData(queues):
         key = attrgetter('job_type')
@@ -158,6 +165,12 @@
             if len(list(specific_jobs)) == 0:
                 continue
             specific_class.preloadJobsData(specific_jobs)
+            specific_jobs_dict = dict(
+                (specific_job.job, specific_job)
+                    for specific_job in specific_jobs)
+            for queue in queue_subset:
+                cache = get_property_cache(queue)
+                cache.specific_job = specific_jobs_dict[queue.job]
 
     @property
     def date_started(self):
@@ -180,6 +193,7 @@
         SQLBase.destroySelf(self)
         specific_job.cleanUp()
         job.destroySelf()
+        self._clear_specific_job_cache()
 
     def manualScore(self, value):
         """See `IBuildQueue`."""

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2012-01-06 15:32:25 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2012-01-06 15:32:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=F0401,E1002
@@ -291,10 +291,12 @@
         # Circular imports.
         from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
         from lp.services.librarian.model import LibraryFileAlias
+        from lp.buildmaster.model.buildfarmjob import BuildFarmJob
         package_builds = load_related(
             PackageBuild, builds, ['package_build_id'])
-        build_farm_jobs = [
-            build.build_farm_job for build in builds]
+        build_farm_jobs = load_related(
+            BuildFarmJob, [build.package_build for build in builds],
+            ['build_farm_job_id'])
         load_related(LibraryFileAlias, build_farm_jobs, ['log_id'])
         archives = load_related(Archive, package_builds, ['archive_id'])
         load_related(Person, archives, ['ownerID'])

=== modified file 'lib/lp/soyuz/browser/builder.py'
--- lib/lp/soyuz/browser/builder.py	2012-01-02 11:21:14 +0000
+++ lib/lp/soyuz/browser/builder.py	2012-01-06 15:32:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for builders."""
@@ -44,7 +44,10 @@
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
-from lp.services.propertycache import cachedproperty
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.services.webapp import (
     ApplicationMenu,
     canonical_url,
@@ -148,11 +151,17 @@
     def builders(self):
         """All active builders"""
         def do_eager_load(builders):
-            # Prefetch the jobs' data.
+            # Populate builders' currentjob cachedproperty.
             queues = IStore(BuildQueue).find(
                 BuildQueue,
                 BuildQueue.builderID.is_in(
                     builder.id for builder in builders))
+            queue_builders = dict(
+                (queue.builderID, queue) for queue in queues)
+            for builder in builders:
+                cache = get_property_cache(builder)
+                cache.currentjob = queue_builders.get(builder.id, None)
+            # Prefetch the jobs' data.
             BuildQueue.preloadSpecificJobData(queues)
 
         return list(DecoratedResultSet(

=== modified file 'lib/lp/soyuz/browser/tests/test_builder.py'
--- lib/lp/soyuz/browser/tests/test_builder.py	2012-01-06 15:32:25 +0000
+++ lib/lp/soyuz/browser/tests/test_builder.py	2012-01-06 15:32:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the lp.soyuz.browser.builder module."""
@@ -67,14 +67,10 @@
 
     layer = LaunchpadFunctionalLayer
 
-    # XXX rvb: the 3 additional queries per build are the result of the calls
-    # to:
-    # - builder.currentjob
-    # - buildqueue.specific_job
-    # These could be converted into cachedproperty and pre-populated in
-    # bulk but several tests assert that the value returned by these
-    # these properties are up to date.  Since they are not really expensive
-    # to compute I'll leave them as regular properties for now.
+    # 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 test_builders_binary_package_build_query_count(self):
         def create_build():
@@ -82,12 +78,13 @@
             queue = build.queueBuild()
             queue.markAsBuilding(build.builder)
 
+        nb_objects = 2
         recorder1, recorder2 = record_two_runs(
-            builders_homepage_render, create_build, 2)
+            builders_homepage_render, create_build, nb_objects)
 
         self.assertThat(
             recorder2,
-            HasQueryCount(LessThan(recorder1.count + 3 * 2 + 1)))
+            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
 
     def test_builders_recipe_build_query_count(self):
         def create_build():
@@ -95,12 +92,13 @@
             queue = build.queueBuild()
             queue.markAsBuilding(build.builder)
 
+        nb_objects = 2
         recorder1, recorder2 = record_two_runs(
-            builders_homepage_render, create_build, 2)
+            builders_homepage_render, create_build, nb_objects)
 
         self.assertThat(
             recorder2,
-            HasQueryCount(LessThan(recorder1.count + 4 * 2 + 1)))
+            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
 
     def test_builders_translation_template_build_query_count(self):
         def create_build():
@@ -114,9 +112,10 @@
             queue = queueset.get(job_id)
             queue.markAsBuilding(self.factory.makeBuilder())
 
+        nb_objects = 2
         recorder1, recorder2 = record_two_runs(
-            builders_homepage_render, create_build, 2)
+            builders_homepage_render, create_build, nb_objects)
 
         self.assertThat(
             recorder2,
-            HasQueryCount(LessThan(recorder1.count + 3 * 2 + 1)))
+            HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))