launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/livefs-build-ordering into lp:launchpad.

Commit message:
Sort cancelled-before-starting livefs builds to the end of the build history.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1424672 in Launchpad itself: "LiveFS builds cancelled before they start sort above other builds in history"
  https://bugs.launchpad.net/launchpad/+bug/1424672

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/livefs-build-ordering/+merge/265320

Sort cancelled-before-starting livefs builds to the end of the build history.

If this is accepted, I'll submit a similar database patch for the index which we can apply live around the same time this is deployed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/livefs-build-ordering into lp:launchpad.

=== modified file 'lib/lp/services/database/stormexpr.py'
--- lib/lp/services/database/stormexpr.py	2015-03-16 16:22:04 +0000
+++ lib/lp/services/database/stormexpr.py	2015-07-20 20:43:48 +0000
@@ -17,6 +17,8 @@
     'get_where_for_reference',
     'IsDistinctFrom',
     'NullCount',
+    'NullsFirst',
+    'NullsLast',
     'rank_by_fti',
     'TryAdvisoryLock',
     'Unnest',
@@ -37,6 +39,7 @@
     NamedFunc,
     Or,
     SQL,
+    SuffixExpr,
     TABLE,
     )
 from storm.info import (
@@ -213,6 +216,18 @@
     oper = " IS DISTINCT FROM "
 
 
+class NullsFirst(SuffixExpr):
+    """Order null values before non-null values."""
+    __slots__ = ()
+    suffix = "NULLS FIRST"
+
+
+class NullsLast(SuffixExpr):
+    """Order null values after non-null values."""
+    __slots__ = ()
+    suffix = "NULLS LAST"
+
+
 def get_where_for_reference(reference, other):
     """Generate a column comparison expression for a reference property.
 

=== modified file 'lib/lp/soyuz/model/livefs.py'
--- lib/lp/soyuz/model/livefs.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/livefs.py	2015-07-20 20:43:48 +0000
@@ -47,7 +47,10 @@
     IMasterStore,
     IStore,
     )
-from lp.services.database.stormexpr import Greatest
+from lp.services.database.stormexpr import (
+    Greatest,
+    NullsLast,
+    )
 from lp.services.features import getFeatureFlag
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.soyuz.interfaces.archive import ArchiveDisabled
@@ -173,9 +176,9 @@
     def builds(self):
         """See `ILiveFS`."""
         order_by = (
-            Desc(Greatest(
+            NullsLast(Desc(Greatest(
                 LiveFSBuild.date_started,
-                LiveFSBuild.date_finished)),
+                LiveFSBuild.date_finished))),
             Desc(LiveFSBuild.date_created),
             Desc(LiveFSBuild.id))
         return self._getBuilds(None, order_by)
@@ -195,9 +198,9 @@
         """See `ILiveFS`."""
         filter_term = (Not(LiveFSBuild.status.is_in(self._pending_states)))
         order_by = (
-            Desc(Greatest(
+            NullsLast(Desc(Greatest(
                 LiveFSBuild.date_started,
-                LiveFSBuild.date_finished)),
+                LiveFSBuild.date_finished))),
             Desc(LiveFSBuild.id))
         return self._getBuilds(filter_term, order_by)
 

=== modified file 'lib/lp/soyuz/tests/test_livefs.py'
--- lib/lp/soyuz/tests/test_livefs.py	2015-05-14 08:56:37 +0000
+++ lib/lp/soyuz/tests/test_livefs.py	2015-07-20 20:43:48 +0000
@@ -234,10 +234,23 @@
         # Change the status of one of the builds and retest.
         builds[0].updateStatus(BuildStatus.BUILDING)
         builds[0].updateStatus(BuildStatus.FULLYBUILT)
-        self.assertEqual(builds[1:] + builds[:1], list(livefs.builds))
+        self.assertEqual(builds, list(livefs.builds))
         self.assertEqual(builds[:1], list(livefs.completed_builds))
         self.assertEqual(builds[1:], list(livefs.pending_builds))
 
+    def test_getBuilds_cancelled_never_started_last(self):
+        # A cancelled build that was never even started sorts to the end.
+        livefs = self.factory.makeLiveFS()
+        fullybuilt = self.factory.makeLiveFSBuild(livefs=livefs)
+        instacancelled = self.factory.makeLiveFSBuild(livefs=livefs)
+        fullybuilt.updateStatus(BuildStatus.BUILDING)
+        fullybuilt.updateStatus(BuildStatus.FULLYBUILT)
+        instacancelled.updateStatus(BuildStatus.CANCELLED)
+        self.assertEqual([fullybuilt, instacancelled], list(livefs.builds))
+        self.assertEqual(
+            [fullybuilt, instacancelled], list(livefs.completed_builds))
+        self.assertEqual([], list(livefs.pending_builds))
+
     def test_getBuilds_privacy(self):
         # The various getBuilds methods exclude builds against invisible
         # archives.
@@ -669,10 +682,11 @@
                 BuildStatus.FULLYBUILT,
                 date_finished=db_livefs.date_created + timedelta(minutes=10))
         livefs = self.webservice.get(livefs["self_link"]).jsonBody()
-        # Builds that have not yet been started are listed first (since DESC
-        # defaults to NULLS FIRST).
-        self.assertEqual(
-            builds[1:] + builds[:1], self.getCollectionLinks(livefs, "builds"))
+        # Builds that have not yet been started are listed last.  This does
+        # mean that pending builds that have never been started are sorted
+        # to the end, but means that builds that were cancelled before
+        # starting don't pollute the start of the collection forever.
+        self.assertEqual(builds, self.getCollectionLinks(livefs, "builds"))
         self.assertEqual(
             builds[:1], self.getCollectionLinks(livefs, "completed_builds"))
         self.assertEqual(
@@ -686,7 +700,7 @@
                 date_finished=db_livefs.date_created + timedelta(minutes=20))
         livefs = self.webservice.get(livefs["self_link"]).jsonBody()
         self.assertEqual(
-            [builds[2], builds[3], builds[1], builds[0]],
+            [builds[1], builds[0], builds[2], builds[3]],
             self.getCollectionLinks(livefs, "builds"))
         self.assertEqual(
             [builds[1], builds[0]],