launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/fix-mailman-excessive-queries into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #666580 getMessageDispositions mailing list xmlrpc api call makes excessive queries
  https://bugs.launchpad.net/bugs/666580


The current implementation of getMessageDispositions on the MailingList xmlrpc api is excessively simplistic in its implementation.  It gets an unlimited number of MessageApproval objects and then iterates over them both updating a status and querying for the team name.

This causes OOPSes like OOPS-1758XMLP341.

I changed the implementation of the IMessageApprovalSet to do the joins and return just the message_id and team names as needed.  I also implemented a new method that does a set based update.

tests:
  message-holds
  mailinglists
-- 
https://code.launchpad.net/~thumper/launchpad/fix-mailman-excessive-queries/+merge/39335
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/fix-mailman-excessive-queries into lp:launchpad/devel.

=== modified file 'lib/lp/registry/doc/message-holds.txt'
--- lib/lp/registry/doc/message-holds.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/registry/doc/message-holds.txt	2010-10-26 02:33:51 +0000
@@ -292,10 +292,10 @@
     ...     print message_id, list_name, subject
 
     >>> def print_messages(status):
-    ...     held_messages = sorted(hold_set.getHeldMessagesWithStatus(status),
-    ...                            key=attrgetter('message_id'))
-    ...     for message_hold in held_messages:
-    ...         print_hold(message_hold)
+    ...     held_messages = sorted(hold_set.getHeldMessagesWithStatus(status))
+    ...     for message_id, team_name in held_messages:
+    ...         held_message = hold_set.getMessageByMessageID(message_id)
+    ...         print_hold(held_message)
 
 Here are all the messages pending approval...
 

=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py	2010-09-17 03:58:05 +0000
+++ lib/lp/registry/interfaces/mailinglist.py	2010-10-26 02:33:51 +0000
@@ -808,6 +808,17 @@
         :rtype: sequence of MessageApproval
         """
 
+    def acknowledgeMessagesWithStats(status):
+        """Acknowledge all the MessageApprovals with the matching status.
+
+        This changes the statuses APPROVAL_PENDING to APPROVED,
+        REJECTION_PENDING to REJECTED and DISCARD_PENDING to DISCARD.  It is
+        illegal to call this function when the status is not one of these
+        states.
+
+        :param status: A PostedMessageStatus enum value.
+        """
+
 
 class IHeldMessageDetails(Interface):
     """Details on a held message.

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-09-17 03:58:05 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-10-26 02:33:51 +0000
@@ -73,6 +73,7 @@
     EmailAddressStatus,
     IEmailAddressSet,
     )
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector,
     MAIN_STORE,
@@ -887,7 +888,34 @@
 
     def getHeldMessagesWithStatus(self, status):
         """See `IMessageApprovalSet`."""
-        return MessageApproval.selectBy(status=status)
+        # Use the master store as the messages will also be acknowledged and
+        # we want to make sure we are acknowledging the same messages that we
+        # iterate over.
+        return IMasterStore(MessageApproval).find(
+            (Message.rfc822msgid, Person.name),
+            MessageApproval.status == status,
+            MessageApproval.message == Message.id,
+            MessageApproval.mailing_list == MailingList.id,
+            MailingList.team == Person.id)
+
+    def acknowledgeMessagesWithStats(self, status):
+        """See `IMessageApprovalSet`."""
+        transitions = {
+            PostedMessageStatus.APPROVAL_PENDING:
+                PostedMessageStatus.APPROVED,
+            PostedMessageStatus.REJECTION_PENDING:
+                PostedMessageStatus.REJECTED,
+            PostedMessageStatus.DISCARD_PENDING:
+                PostedMessageStatus.DISCARDED,
+            }
+        try:
+            next_state = transitions[status]
+        except KeyError:
+            raise AssertionError(
+                'Not an acknowledgeable state: %s' % status)
+        approvals = IMasterStore(MessageApproval).find(
+            MessageApproval, MessageApproval.status == status)
+        approvals.set(status=next_state)
 
 
 class HeldMessageDetails:

=== modified file 'lib/lp/registry/tests/mailinglists_helper.py'
--- lib/lp/registry/tests/mailinglists_helper.py	2010-09-20 19:21:57 +0000
+++ lib/lp/registry/tests/mailinglists_helper.py	2010-10-26 02:33:51 +0000
@@ -263,15 +263,10 @@
             mailing_list.transitionToStatus(MailingListStatus.ACTIVE)
         # Simulate acknowledging held messages.
         message_set = getUtility(IMessageApprovalSet)
-        message_ids = set()
         for status in (PostedMessageStatus.APPROVAL_PENDING,
                        PostedMessageStatus.REJECTION_PENDING,
                        PostedMessageStatus.DISCARD_PENDING):
-            for message in message_set.getHeldMessagesWithStatus(status):
-                message_ids.add(message.message_id)
-        for message_id in message_ids:
-            message = message_set.getMessageByMessageID(message_id)
-            message.acknowledge()
+            message_set.acknowledgeMessagesWithStats(status)
 
 
 mailman = MailmanStub()

=== modified file 'lib/lp/registry/xmlrpc/mailinglist.py'
--- lib/lp/registry/xmlrpc/mailinglist.py	2010-10-03 15:30:06 +0000
+++ lib/lp/registry/xmlrpc/mailinglist.py	2010-10-26 02:33:51 +0000
@@ -275,9 +275,8 @@
             (PostedMessageStatus.DISCARD_PENDING, 'discard'),
             )
         for status, disposition in status_dispositions:
-            for held_message in message_set.getHeldMessagesWithStatus(status):
-                held_message.acknowledge()
-                response[held_message.message_id] = (
-                    removeSecurityProxy(held_message.mailing_list.team).name,
-                    disposition)
+            held_messages = message_set.getHeldMessagesWithStatus(status)
+            for message_id, team_name in held_messages:
+                response[message_id] = (team_name, disposition)
+            message_set.acknowledgeMessagesWithStats(status)
         return response