launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-cache-eta into lp:launchpad.

Commit message:
Cache expensive SnapBuild.eta property.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1613652 in Launchpad itself: "timeouts when triggering snap builds"
  https://bugs.launchpad.net/launchpad/+bug/1613652

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-cache-eta/+merge/303229

Cache expensive SnapBuild.eta property.  It's used multiple times when rendering Snap:+index, for instance.  This regressed in r18036 when moving these properties into the model for use by an API method, and it's a bit dodgy that we have to be careful to clear the property cache when build states change; however, it's only used by the API and by browser code, so should be OK.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-cache-eta into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2016-08-12 12:56:41 +0000
+++ lib/lp/snappy/model/snapbuild.py	2016-08-18 09:59:01 +0000
@@ -55,6 +55,7 @@
     LibraryFileAlias,
     LibraryFileContent,
     )
+from lp.services.propertycache import cachedproperty
 from lp.snappy.interfaces.snap import ISnapSet
 from lp.snappy.interfaces.snapbuild import (
     CannotScheduleStoreUpload,
@@ -348,7 +349,7 @@
     def getFileUrls(self):
         return [self.lfaUrl(lfa) for _, lfa, _ in self.getFiles()]
 
-    @property
+    @cachedproperty
     def eta(self):
         """The datetime when the build job is estimated to complete.
 

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2016-08-12 12:56:41 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2016-08-18 09:59:01 +0000
@@ -34,6 +34,7 @@
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
+from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.webapp.publisher import canonical_url
 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
@@ -50,6 +51,7 @@
     login,
     logout,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.dbuser import dbuser
@@ -58,6 +60,7 @@
     LaunchpadZopelessLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import webservice_for_person
 
 
@@ -329,8 +332,17 @@
         self.build.queueBuild()
         self.assertIsNone(self.build.eta)
         self.factory.makeBuilder(processors=[self.build.processor])
+        clear_property_cache(self.build)
         self.assertIsNotNone(self.build.eta)
 
+    def test_eta_cached(self):
+        # The expensive completion time estimate is cached.
+        self.build.queueBuild()
+        self.build.eta
+        with StormStatementRecorder() as recorder:
+            self.build.eta
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_estimate(self):
         # SnapBuild.estimate returns True until the job is completed.
         self.build.queueBuild()
@@ -338,6 +350,7 @@
         self.build.updateStatus(BuildStatus.BUILDING)
         self.assertTrue(self.build.estimate)
         self.build.updateStatus(BuildStatus.FULLYBUILT)
+        clear_property_cache(self.build)
         self.assertFalse(self.build.estimate)
 
     def setUpStoreUpload(self):