launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-777334 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #777344 in Launchpad itself: "Person:+archivesubscriptions timeouts (late evaluation)"
  https://bugs.launchpad.net/launchpad/+bug/777344

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-777334/+merge/60193

This branch is a fix for bug 777344: Person:+archivesubscriptions timeouts (late evaluation)

I did not dive deep enough into the code to figure out if we could replace the repeated queries by some clever prejoining.

But I noticed that the queries issued in getBySubscriber() had a somewhat odd sub-query:

SELECT $INT
FROM
  (SELECT ArchiveSubscriber.archive, ArchiveSubscriber.cancelled_by, ArchiveSubscriber.date_cancelled, ArchiveSubscriber.date_created, ArchiveSubscriber.date_expires, ArchiveSubscriber.description, ArchiveSubscriber.id, ArchiveSubscriber.registrant, ArchiveSubscriber.status, ArchiveSubscriber.subscriber
   FROM ArchiveSubscriber
   WHERE ArchiveSubscriber.subscriber IN
       (SELECT TeamParticipation.team
        FROM ArchiveSubscriber, TeamParticipation
        WHERE TeamParticipation.person = $INT
          AND TeamParticipation.team = ArchiveSubscriber.subscriber)
     AND ArchiveSubscriber.archive = $INT
     AND ArchiveSubscriber.status = $INTLIMIT $INT) AS "_tmp" LIMIT $INT

The sub-SELECT TeamParticipation.team FROM ArchiveSubscriber... queries the table ArchiveSubscriber in order to find rows for teams where the given user is a member -- but rows from the same table are used in the main SELECT, so I replaced the clause "ArchiveSubscriber.subscriber IN (SELECT TeamParticipation.team...)" by a JOIN.

I also noticed that the methods getBySubscriber() and getBySubscriberWithActiveToken() are identical, except for different result data and for different parameters, so I refactored them.

Running the new queries on staging shows that the execution time is in the order of 1msec, hence fast enough for repeated calls.

I also noticed that the doc string of IArchiveSubscriber.getBySubscriber() was slightly outdated.
(the remaining diff output for interfaces/archivesubscriber.py is from lint removal.)

tests: ./bin/test soyuz -vvt browser.tests.archivesubscription-views.txt -vvt archivesubscriber.txt -vvt archiveauthtoken.txt

no lint


-- 
https://code.launchpad.net/~adeuring/launchpad/bug-777334/+merge/60193
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-777334 into lp:launchpad.

=== modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
--- lib/lp/soyuz/interfaces/archivesubscriber.py	2011-03-29 04:54:48 +0000
+++ lib/lp/soyuz/interfaces/archivesubscriber.py	2011-05-06 14:37:36 +0000
@@ -11,7 +11,7 @@
     'ArchiveSubscriptionError',
     'IArchiveSubscriber',
     'IArchiveSubscriberSet',
-    'IPersonalArchiveSubscription'
+    'IPersonalArchiveSubscription',
     ]
 
 from lazr.restful.declarations import (
@@ -65,7 +65,7 @@
         vocabulary='ValidPersonOrTeam',
         description=_("The person who is subscribed.")))
     subscriber_id = Attribute('database ID of the subscriber.')
-    
+
     date_expires = exported(Datetime(
         title=_("Date of Expiration"), required=False,
         description=_("The timestamp when the subscription will expire.")))
@@ -99,6 +99,7 @@
         :rtype: `storm.store.ResultSet`
         """
 
+
 class IArchiveSubscriberEdit(Interface):
     """An interface for launchpad.Edit ops on archive subscribers."""
 
@@ -129,9 +130,6 @@
             the results to that particular archive.
         :param current_only: Whether the result should only include current
             subscriptions (which is the default).
-        :param return_tokens: Indicates whether the tokens for the given
-            subscribers subscriptions should be included in the resultset.
-            By default the tokens are not included in the resultset.
         """
 
     def getBySubscriberWithActiveToken(subscriber, archive=None):

=== modified file 'lib/lp/soyuz/model/archivesubscriber.py'
--- lib/lp/soyuz/model/archivesubscriber.py	2010-08-23 17:26:42 +0000
+++ lib/lp/soyuz/model/archivesubscriber.py	2011-05-06 14:37:36 +0000
@@ -14,7 +14,7 @@
     And,
     Desc,
     LeftJoin,
-    Select,
+    Join,
     )
 from storm.locals import (
     DateTime,
@@ -135,13 +135,44 @@
 class ArchiveSubscriberSet:
     """See `IArchiveSubscriberSet`."""
 
-    def getBySubscriber(self, subscriber, archive=None, current_only=True):
-        """See `IArchiveSubscriberSet`."""
+    def _getBySubscriber(self, subscriber, archive, current_only,
+                         with_active_tokens):
+        """Return all the subscriptions for a person.
 
+        :param subscriber: An `IPerson` for whom to return all
+            `ArchiveSubscriber` records.
+        :param archive: An optional `IArchive` which restricts
+            the results to that particular archive.
+        :param current_only: Whether the result should only include current
+            subscriptions (which is the default).
+        :param with_active_tokens: Indicates whether the tokens for the given
+            subscribers subscriptions should be included in the resultset.
+            By default the tokens are not included in the resultset.
+^       """
         # Grab the extra Storm expressions, for this query,
         # depending on the params:
         extra_exprs = self._getExprsForSubscriptionQueries(
             archive, current_only)
+        origin = [
+            ArchiveSubscriber,
+            Join(
+                TeamParticipation,
+                TeamParticipation.teamID == ArchiveSubscriber.subscriber_id)]
+
+        if with_active_tokens:
+            result_row = (ArchiveSubscriber, ArchiveAuthToken)
+            # We need a left join with ArchiveSubscriber as
+            # the origin:
+            origin.append(
+                LeftJoin(
+                    ArchiveAuthToken,
+                    And(
+                        ArchiveAuthToken.archive_id ==
+                            ArchiveSubscriber.archive_id,
+                        ArchiveAuthToken.person_id == subscriber.id,
+                        ArchiveAuthToken.date_deactivated == None)))
+        else:
+            result_row = ArchiveSubscriber
 
         # Set the main expression to find all the subscriptions for
         # which the subscriber is a direct subscriber OR is a member
@@ -151,40 +182,18 @@
         # showing that each person is a member of the "team" that
         # consists of themselves.
         store = Store.of(subscriber)
-        return store.find(
-            ArchiveSubscriber,
-            ArchiveSubscriber.subscriber_id.is_in(
-                self._getTeamsWithSubscriptionsForUser(
-                    subscriber)),
+        return store.using(*origin).find(
+            result_row,
+            TeamParticipation.personID == subscriber.id,
             *extra_exprs).order_by(Desc(ArchiveSubscriber.date_created))
 
+    def getBySubscriber(self, subscriber, archive=None, current_only=True):
+        """See `IArchiveSubscriberSet`."""
+        return self._getBySubscriber(subscriber, archive, current_only, False)
+
     def getBySubscriberWithActiveToken(self, subscriber, archive=None):
         """See `IArchiveSubscriberSet`."""
-
-        # We need a left join with ArchiveSubscriber as
-        # the origin:
-        origin = [
-            ArchiveSubscriber,
-            LeftJoin(
-                ArchiveAuthToken,
-                And(
-                    ArchiveAuthToken.archive_id ==
-                        ArchiveSubscriber.archive_id,
-                    ArchiveAuthToken.person_id == subscriber.id,
-                    ArchiveAuthToken.date_deactivated == None))]
-
-        # Grab the extra Storm expressions, for this query,
-        # depending on the params:
-        extra_exprs = self._getExprsForSubscriptionQueries(
-            archive)
-
-        store = Store.of(subscriber)
-        return store.using(*origin).find(
-            (ArchiveSubscriber, ArchiveAuthToken),
-            ArchiveSubscriber.subscriber_id.is_in(
-                self._getTeamsWithSubscriptionsForUser(
-                    subscriber)),
-            *extra_exprs).order_by(Desc(ArchiveSubscriber.date_created))
+        return self._getBySubscriber(subscriber, archive, True, True)
 
     def getByArchive(self, archive, current_only=True):
         """See `IArchiveSubscriberSet`."""
@@ -217,19 +226,3 @@
                 ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT)
 
         return extra_exprs
-
-    def _getTeamsWithSubscriptionsForUser(self, subscriber):
-        """Return a subselect that defines all the teams the subscriber
-        is a member of.that have subscriptions.
-
-        Just to keep the code DRY.
-        """
-        # Include subscriptions for teams of which the subscriber is a
-        # member. First create a subselect to capture all the teams that are
-        # subscribed to archives AND the user is a member of:
-        return Select(
-            TeamParticipation.teamID,
-            where=And(
-                TeamParticipation.personID == subscriber.id,
-                TeamParticipation.teamID ==
-                    ArchiveSubscriber.subscriber_id))