Launchpad itself

Merge lp:~thumper/launchpad/fix-mailman-excessive-queries into lp:launchpad

  1. fix-mailman-excessive-queries
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11803
Proposed branch: lp:~thumper/launchpad/fix-mailman-excessive-queries
Merge into: lp:launchpad
Diff against target: 127 lines (+49/-16)
5 files modified
lib/lp/registry/doc/message-holds.txt (+4/-4)
lib/lp/registry/interfaces/mailinglist.py (+11/-0)
lib/lp/registry/model/mailinglist.py (+29/-1)
lib/lp/registry/tests/mailinglists_helper.py (+1/-6)
lib/lp/registry/xmlrpc/mailinglist.py (+4/-5)
To merge this branch: bzr merge lp:~thumper/launchpad/fix-mailman-excessive-queries
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+39335@code.launchpad.net

Commit message

Make getMessageDispositions much more efficient in the number of DB queries.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Really nice branch Tim.

I think you have a typo in your new method name that got propagated.
acknowledgeMessagesWithStats ->
acknowledgeMessagesWithStatus

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 03:53:45 +0000
@@ -292,10 +292,10 @@
292 ... print message_id, list_name, subject292 ... print message_id, list_name, subject
293293
294 >>> def print_messages(status):294 >>> def print_messages(status):
295 ... held_messages = sorted(hold_set.getHeldMessagesWithStatus(status),295 ... held_messages = sorted(hold_set.getHeldMessagesWithStatus(status))
296 ... key=attrgetter('message_id'))296 ... for message_id, team_name in held_messages:
297 ... for message_hold in held_messages:297 ... held_message = hold_set.getMessageByMessageID(message_id)
298 ... print_hold(message_hold)298 ... print_hold(held_message)
299299
300Here are all the messages pending approval...300Here are all the messages pending approval...
301301
302302
=== 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 03:53:45 +0000
@@ -808,6 +808,17 @@
808 :rtype: sequence of MessageApproval808 :rtype: sequence of MessageApproval
809 """809 """
810810
811 def acknowledgeMessagesWithStatus(status):
812 """Acknowledge all the MessageApprovals with the matching status.
813
814 This changes the statuses APPROVAL_PENDING to APPROVED,
815 REJECTION_PENDING to REJECTED and DISCARD_PENDING to DISCARD. It is
816 illegal to call this function when the status is not one of these
817 states.
818
819 :param status: A PostedMessageStatus enum value.
820 """
821
811822
812class IHeldMessageDetails(Interface):823class IHeldMessageDetails(Interface):
813 """Details on a held message.824 """Details on a held message.
814825
=== 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 03:53:45 +0000
@@ -73,6 +73,7 @@
73 EmailAddressStatus,73 EmailAddressStatus,
74 IEmailAddressSet,74 IEmailAddressSet,
75 )75 )
76from canonical.launchpad.interfaces.lpstorm import IMasterStore
76from canonical.launchpad.webapp.interfaces import (77from canonical.launchpad.webapp.interfaces import (
77 IStoreSelector,78 IStoreSelector,
78 MAIN_STORE,79 MAIN_STORE,
@@ -887,7 +888,34 @@
887888
888 def getHeldMessagesWithStatus(self, status):889 def getHeldMessagesWithStatus(self, status):
889 """See `IMessageApprovalSet`."""890 """See `IMessageApprovalSet`."""
890 return MessageApproval.selectBy(status=status)891 # Use the master store as the messages will also be acknowledged and
892 # we want to make sure we are acknowledging the same messages that we
893 # iterate over.
894 return IMasterStore(MessageApproval).find(
895 (Message.rfc822msgid, Person.name),
896 MessageApproval.status == status,
897 MessageApproval.message == Message.id,
898 MessageApproval.mailing_list == MailingList.id,
899 MailingList.team == Person.id)
900
901 def acknowledgeMessagesWithStatus(self, status):
902 """See `IMessageApprovalSet`."""
903 transitions = {
904 PostedMessageStatus.APPROVAL_PENDING:
905 PostedMessageStatus.APPROVED,
906 PostedMessageStatus.REJECTION_PENDING:
907 PostedMessageStatus.REJECTED,
908 PostedMessageStatus.DISCARD_PENDING:
909 PostedMessageStatus.DISCARDED,
910 }
911 try:
912 next_state = transitions[status]
913 except KeyError:
914 raise AssertionError(
915 'Not an acknowledgeable state: %s' % status)
916 approvals = IMasterStore(MessageApproval).find(
917 MessageApproval, MessageApproval.status == status)
918 approvals.set(status=next_state)
891919
892920
893class HeldMessageDetails:921class HeldMessageDetails:
894922
=== 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 03:53:45 +0000
@@ -263,15 +263,10 @@
263 mailing_list.transitionToStatus(MailingListStatus.ACTIVE)263 mailing_list.transitionToStatus(MailingListStatus.ACTIVE)
264 # Simulate acknowledging held messages.264 # Simulate acknowledging held messages.
265 message_set = getUtility(IMessageApprovalSet)265 message_set = getUtility(IMessageApprovalSet)
266 message_ids = set()
267 for status in (PostedMessageStatus.APPROVAL_PENDING,266 for status in (PostedMessageStatus.APPROVAL_PENDING,
268 PostedMessageStatus.REJECTION_PENDING,267 PostedMessageStatus.REJECTION_PENDING,
269 PostedMessageStatus.DISCARD_PENDING):268 PostedMessageStatus.DISCARD_PENDING):
270 for message in message_set.getHeldMessagesWithStatus(status):269 message_set.acknowledgeMessagesWithStatus(status)
271 message_ids.add(message.message_id)
272 for message_id in message_ids:
273 message = message_set.getMessageByMessageID(message_id)
274 message.acknowledge()
275270
276271
277mailman = MailmanStub()272mailman = MailmanStub()
278273
=== 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 03:53:45 +0000
@@ -275,9 +275,8 @@
275 (PostedMessageStatus.DISCARD_PENDING, 'discard'),275 (PostedMessageStatus.DISCARD_PENDING, 'discard'),
276 )276 )
277 for status, disposition in status_dispositions:277 for status, disposition in status_dispositions:
278 for held_message in message_set.getHeldMessagesWithStatus(status):278 held_messages = message_set.getHeldMessagesWithStatus(status)
279 held_message.acknowledge()279 for message_id, team_name in held_messages:
280 response[held_message.message_id] = (280 response[message_id] = (team_name, disposition)
281 removeSecurityProxy(held_message.mailing_list.team).name,281 message_set.acknowledgeMessagesWithStatus(status)
282 disposition)
283 return response282 return response