Launchpad itself

Merge lp:~lifeless/launchpad/bug-618849 into lp:launchpad

  1. bug-618849
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11472
Proposed branch: lp:~lifeless/launchpad/bug-618849
Merge into: lp:launchpad
Diff against target: 388 lines (+144/-27)
11 files modified
lib/canonical/launchpad/database/message.py (+1/-1)
lib/lp/bugs/browser/bug.py (+8/-6)
lib/lp/bugs/browser/bugtask.py (+1/-1)
lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt (+1/-1)
lib/lp/bugs/configure.zcml (+1/-0)
lib/lp/bugs/doc/bug.txt (+6/-2)
lib/lp/bugs/interfaces/bug.py (+10/-0)
lib/lp/bugs/model/bug.py (+50/-9)
lib/lp/bugs/model/bugattachment.py (+13/-2)
lib/lp/bugs/tests/test_bugs_webservice.py (+52/-4)
lib/lp/testing/sampledata.py (+1/-1)
To merge this branch: bzr merge lp:~lifeless/launchpad/bug-618849
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Jelmer Vernooij Pending
Review via email: mp+34011@code.launchpad.net

Commit message

Remove O(N^2) scaling from the Bug.attachments API call by injecting IIndexedMessage objects into the BugAttachments.

Description of the change

Reduce the query count for bug/attachments API calls: inject an IIndexedMessage into the bug attachment when we query, rather than doing O(N^2) work later. Also, to make my test easier to write, fixed up the LibraryFileAlias lookups to be eager loaded.

Net result - 23 SQL calls to answer the API (down from 38 for a bug with 2 attachments when I started testing this). There is a lurking surprise when indexed_messages becomes too expensive to query; I think we probably want the index in the DB rather than calculating at runtime, however that is a separate bridge to cross in the future.

The implementation is a little convoluted due to:
 - needing to index a *non db class* as the answer for bugattachment.message (https://answers.edge.launchpad.net/lazr.restful/+question/123212)
 - lazr.restful not being able to replace an unexported attribute. (bug 625102)

The former point is also potentially solvable via lazr restful, but I'm not sure its a bug, so its just a question for now.

I've done some driveby cleanups to sample data in the test I modified too.

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

As we discussed on IRC, I'm fine with all the code, but asked for a few changes in the docs.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/database/message.py'
--- lib/canonical/launchpad/database/message.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/database/message.py 2010-08-30 05:20:59 +0000
@@ -127,7 +127,7 @@
127 chunks = SQLMultipleJoin('MessageChunk', joinColumn='message')127 chunks = SQLMultipleJoin('MessageChunk', joinColumn='message')
128 raw = ForeignKey(foreignKey='LibraryFileAlias', dbName='raw',128 raw = ForeignKey(foreignKey='LibraryFileAlias', dbName='raw',
129 default=None)129 default=None)
130 bugattachments = SQLMultipleJoin('BugAttachment', joinColumn='message')130 bugattachments = SQLMultipleJoin('BugAttachment', joinColumn='_message')
131131
132 def __iter__(self):132 def __iter__(self):
133 """See IMessage.__iter__"""133 """See IMessage.__iter__"""
134134
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/bug.py 2010-08-30 05:20:59 +0000
@@ -519,7 +519,7 @@
519 'file': ProxiedLibraryFileAlias(519 'file': ProxiedLibraryFileAlias(
520 attachment.libraryfile, attachment),520 attachment.libraryfile, attachment),
521 }521 }
522 for attachment in self.context.attachments522 for attachment in self.context.attachments_unpopulated
523 if attachment.type != BugAttachmentType.PATCH]523 if attachment.type != BugAttachmentType.PATCH]
524524
525 @property525 @property
@@ -531,7 +531,7 @@
531 'file': ProxiedLibraryFileAlias(531 'file': ProxiedLibraryFileAlias(
532 attachment.libraryfile, attachment),532 attachment.libraryfile, attachment),
533 }533 }
534 for attachment in self.context.attachments534 for attachment in self.context.attachments_unpopulated
535 if attachment.type == BugAttachmentType.PATCH]535 if attachment.type == BugAttachmentType.PATCH]
536536
537537
@@ -877,15 +877,17 @@
877 if bug.security_related:877 if bug.security_related:
878 text.append('security: yes')878 text.append('security: yes')
879879
880 patches = []
880 text.append('attachments: ')881 text.append('attachments: ')
881 for attachment in bug.attachments:882 for attachment in bug.attachments_unpopulated:
882 if attachment.type != BugAttachmentType.PATCH:883 if attachment.type != BugAttachmentType.PATCH:
883 text.append(' %s' % self.attachment_text(attachment))884 text.append(' %s' % self.attachment_text(attachment))
885 else:
886 patches.append(attachment)
884887
885 text.append('patches: ')888 text.append('patches: ')
886 for attachment in bug.attachments:889 for attachment in patches:
887 if attachment.type == BugAttachmentType.PATCH:890 text.append(' %s' % self.attachment_text(attachment))
888 text.append(' %s' % self.attachment_text(attachment))
889891
890 text.append('tags: %s' % ' '.join(bug.tags))892 text.append('tags: %s' % ' '.join(bug.tags))
891893
892894
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/browser/bugtask.py 2010-08-30 05:20:59 +0000
@@ -310,7 +310,7 @@
310 """310 """
311 chunks = bugtask.bug.getMessageChunks()311 chunks = bugtask.bug.getMessageChunks()
312 comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)312 comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)
313 for attachment in bugtask.bug.attachments:313 for attachment in bugtask.bug.attachments_unpopulated:
314 message_id = attachment.message.id314 message_id = attachment.message.id
315 # All attachments are related to a message, so we can be315 # All attachments are related to a message, so we can be
316 # sure that the BugComment is already created.316 # sure that the BugComment is already created.
317317
=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2010-02-23 15:34:15 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2010-08-30 05:20:59 +0000
@@ -284,7 +284,7 @@
284The attachments got added, with the charsets preserved, and the one that284The attachments got added, with the charsets preserved, and the one that
285didn't specify a description got an autogenerated one.285didn't specify a description got an autogenerated one.
286286
287 >>> for attachment in filebug_view.added_bug.attachments:287 >>> for attachment in filebug_view.added_bug.attachments_unpopulated:
288 ... print "Filename: %s" % attachment.libraryfile.filename288 ... print "Filename: %s" % attachment.libraryfile.filename
289 ... print "Content type: %s" % attachment.libraryfile.mimetype289 ... print "Content type: %s" % attachment.libraryfile.mimetype
290 ... print "Description: %s" % attachment.title290 ... print "Description: %s" % attachment.title
291291
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-08-26 12:50:12 +0000
+++ lib/lp/bugs/configure.zcml 2010-08-30 05:20:59 +0000
@@ -626,6 +626,7 @@
626 cve_links626 cve_links
627 duplicates627 duplicates
628 attachments628 attachments
629 attachments_unpopulated
629 questions630 questions
630 specifications631 specifications
631 followup_subject632 followup_subject
632633
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2010-08-02 17:48:13 +0000
+++ lib/lp/bugs/doc/bug.txt 2010-08-30 05:20:59 +0000
@@ -1130,8 +1130,12 @@
1130It's done in a way that we only issue two queries to fetch all this1130It's done in a way that we only issue two queries to fetch all this
1131information, too:1131information, too:
11321132
1133 >>> len(CursorWrapper.last_executed_sql) - queries1133XXX: bug=https://bugs.launchpad.net/storm/+bug/619017 means that this sometimes
1134 21134does 3 queries, depending on the precise state of the storm cache. To avoid
1135spurious failures it has been changed to tolerate this additional query.
1136
1137 >>> len(CursorWrapper.last_executed_sql) - queries <= 3
1138 True
11351139
1136Bugs have a special attribute, `indexed_messages` which returns the collection1140Bugs have a special attribute, `indexed_messages` which returns the collection
1137of messages, each decorated with the index of that message in its context1141of messages, each decorated with the index of that message in its context
11381142
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-08-30 05:20:59 +0000
@@ -279,6 +279,16 @@
279 CollectionField(279 CollectionField(
280 title=_("MultiJoin of bugs which are dupes of this one."),280 title=_("MultiJoin of bugs which are dupes of this one."),
281 value_type=BugField(), readonly=True))281 value_type=BugField(), readonly=True))
282 # See lp.bugs.model.bug.Bug.attachments for why there are two similar
283 # properties here.
284 # attachments_unpopulated would more naturally be attachments, and
285 # attachments be attachments_prepopulated, but lazr.resful cannot
286 # export over a non-exported attribute in an interface.
287 # https://bugs.launchpad.net/lazr.restful/+bug/625102
288 attachments_unpopulated = CollectionField(
289 title=_("List of bug attachments."),
290 value_type=Reference(schema=IBugAttachment),
291 readonly=True)
282 attachments = exported(292 attachments = exported(
283 CollectionField(293 CollectionField(
284 title=_("List of bug attachments."),294 title=_("List of bug attachments."),
285295
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-08-25 14:41:20 +0000
+++ lib/lp/bugs/model/bug.py 2010-08-30 05:20:59 +0000
@@ -72,6 +72,7 @@
7272
73from canonical.cachedproperty import (73from canonical.cachedproperty import (
74 cachedproperty,74 cachedproperty,
75 cache_property,
75 clear_property,76 clear_property,
76 )77 )
77from canonical.config import config78from canonical.config import config
@@ -82,6 +83,7 @@
82 SQLBase,83 SQLBase,
83 sqlvalues,84 sqlvalues,
84 )85 )
86from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
85from canonical.launchpad.database.librarian import LibraryFileAlias87from canonical.launchpad.database.librarian import LibraryFileAlias
86from canonical.launchpad.database.message import (88from canonical.launchpad.database.message import (
87 Message,89 Message,
@@ -1476,7 +1478,9 @@
1476 self.who_made_private = None1478 self.who_made_private = None
1477 self.date_made_private = None1479 self.date_made_private = None
14781480
1479 for attachment in self.attachments:1481 # XXX: This should be a bulk update. RBC 20100827
1482 # bug=https://bugs.edge.launchpad.net/storm/+bug/625071
1483 for attachment in self.attachments_unpopulated:
1480 attachment.libraryfile.restricted = private1484 attachment.libraryfile.restricted = private
14811485
1482 # Correct the heat for the bug immediately, so that we don't have1486 # Correct the heat for the bug immediately, so that we don't have
@@ -1728,19 +1732,56 @@
1728 task.target.recalculateBugHeatCache()1732 task.target.recalculateBugHeatCache()
1729 store.flush()1733 store.flush()
17301734
1731 @property1735 def _attachments_query(self):
1732 def attachments(self):1736 """Helper for the attachments* properties."""
1733 """See `IBug`."""1737 # bug attachments with no LibraryFileContent have been deleted - the
1734 # We omit those bug attachments that do not have a1738 # garbo_daily run will remove the LibraryFileAlias asynchronously.
1735 # LibraryFileContent record in order to avoid OOPSes as1739 # See bug 542274 for more details.
1736 # mentioned in bug 542274. These bug attachments will be
1737 # deleted anyway during the next garbo_daily run.
1738 store = Store.of(self)1740 store = Store.of(self)
1739 return store.find(1741 return store.find(
1740 BugAttachment, BugAttachment.bug == self,1742 (BugAttachment, LibraryFileAlias),
1743 BugAttachment.bug == self,
1741 BugAttachment.libraryfile == LibraryFileAlias.id,1744 BugAttachment.libraryfile == LibraryFileAlias.id,
1742 LibraryFileAlias.content != None).order_by(BugAttachment.id)1745 LibraryFileAlias.content != None).order_by(BugAttachment.id)
17431746
1747 @property
1748 def attachments(self):
1749 """See `IBug`.
1750
1751 This property does eager loading of the index_messages so that the API
1752 which wants the message_link for the attachment can answer that without
1753 O(N^2) overhead. As such it is moderately expensive to call (it
1754 currently retrieves all messages before any attachments, and does this
1755 when attachments is evaluated, not when the resultset is processed).
1756 """
1757 message_to_indexed = {}
1758 for message in self.indexed_messages:
1759 message_to_indexed[message.id] = message
1760 def set_indexed_message(row):
1761 attachment = row[0]
1762 # row[1] - the LibraryFileAlias is now in the storm cache and
1763 # will be found without a query when dereferenced.
1764 indexed_message = message_to_indexed.get(attachment._messageID)
1765 if indexed_message is not None:
1766 cache_property(attachment, '_message_cached', indexed_message)
1767 return attachment
1768 rawresults = self._attachments_query()
1769 return DecoratedResultSet(rawresults, set_indexed_message)
1770
1771 @property
1772 def attachments_unpopulated(self):
1773 """See `IBug`.
1774
1775 This version does not pre-lookup messages and LibraryFileAliases.
1776
1777 The regular 'attachments' property does prepopulation because it is
1778 exposed in the API.
1779 """
1780 # Grab the attachment only; the LibraryFileAlias will be eager loaded.
1781 return DecoratedResultSet(
1782 self._attachments_query(),
1783 operator.itemgetter(0))
1784
17441785
1745class BugSet:1786class BugSet:
1746 """See BugSet."""1787 """See BugSet."""
17471788
=== modified file 'lib/lp/bugs/model/bugattachment.py'
--- lib/lp/bugs/model/bugattachment.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugattachment.py 2010-08-30 05:20:59 +0000
@@ -19,6 +19,7 @@
19from zope.event import notify19from zope.event import notify
20from zope.interface import implements20from zope.interface import implements
2121
22from canonical.cachedproperty import cachedproperty
22from canonical.database.enumcol import EnumCol23from canonical.database.enumcol import EnumCol
23from canonical.database.sqlbase import SQLBase24from canonical.database.sqlbase import SQLBase
24from lp.app.errors import NotFoundError25from lp.app.errors import NotFoundError
@@ -46,9 +47,19 @@
46 foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)47 foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)
47 data = ForeignKey(48 data = ForeignKey(
48 foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)49 foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)
49 message = ForeignKey(50 _message = ForeignKey(
50 foreignKey='Message', dbName='message', notNull=True)51 foreignKey='Message', dbName='message', notNull=True)
5152
53 @cachedproperty('_message_cached')
54 def message(self):
55 """This is a cachedproperty to allow message to be an IIndexedMessage.
56
57 This is needed for the bug/attachments API call which needs to index
58 an IIndexedMessage rather than a simple DB model IMessage. See
59 Bug.attachments where the injection occurs.
60 """
61 return self._message
62
52 @property63 @property
53 def is_patch(self):64 def is_patch(self):
54 """See IBugAttachment."""65 """See IBugAttachment."""
@@ -99,7 +110,7 @@
99 attach_type = IBugAttachment['type'].default110 attach_type = IBugAttachment['type'].default
100 attachment = BugAttachment(111 attachment = BugAttachment(
101 bug=bug, libraryfile=filealias, type=attach_type, title=title,112 bug=bug, libraryfile=filealias, type=attach_type, title=title,
102 message=message)113 _message=message)
103 # canonial_url(attachment) (called by notification subscribers114 # canonial_url(attachment) (called by notification subscribers
104 # to generate the download URL of the attachments) blows up if115 # to generate the download URL of the attachments) blows up if
105 # attachment.id is not (yet) set.116 # attachment.id is not (yet) set.
106117
=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py 2010-08-30 05:20:59 +0000
@@ -10,6 +10,11 @@
10from BeautifulSoup import BeautifulSoup10from BeautifulSoup import BeautifulSoup
11from lazr.lifecycle.interfaces import IDoNotSnapshot11from lazr.lifecycle.interfaces import IDoNotSnapshot
12from simplejson import dumps12from simplejson import dumps
13from testtools.matchers import (
14 Equals,
15 LessThan,
16 MatchesAny,
17 )
13from zope.component import getMultiAdapter18from zope.component import getMultiAdapter
1419
15from canonical.launchpad.ftests import (20from canonical.launchpad.ftests import (
@@ -19,10 +24,19 @@
19from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller24from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
20from canonical.launchpad.webapp import snapshot25from canonical.launchpad.webapp import snapshot
21from canonical.launchpad.webapp.servers import LaunchpadTestRequest26from canonical.launchpad.webapp.servers import LaunchpadTestRequest
22from canonical.testing import DatabaseFunctionalLayer27from canonical.testing import (
28 DatabaseFunctionalLayer,
29 LaunchpadFunctionalLayer,
30 )
23from lp.bugs.browser.bugtask import get_comments_for_bugtask31from lp.bugs.browser.bugtask import get_comments_for_bugtask
24from lp.bugs.interfaces.bug import IBug32from lp.bugs.interfaces.bug import IBug
25from lp.testing import TestCaseWithFactory33from lp.testing import TestCaseWithFactory
34from lp.testing.matchers import HasQueryCount
35from lp.testing.sampledata import (
36 ADMIN_EMAIL,
37 USER_EMAIL,
38 )
39from lp.testing._webservice import QueryCollector
2640
2741
28class TestBugDescriptionRepresentation(TestCaseWithFactory):42class TestBugDescriptionRepresentation(TestCaseWithFactory):
@@ -31,7 +45,7 @@
3145
32 def setUp(self):46 def setUp(self):
33 TestCaseWithFactory.setUp(self)47 TestCaseWithFactory.setUp(self)
34 login('foo.bar@canonical.com')48 login(ADMIN_EMAIL)
35 # Make two bugs, one whose description points to the other, so it will49 # Make two bugs, one whose description points to the other, so it will
36 # get turned into a HTML link.50 # get turned into a HTML link.
37 self.bug_one = self.factory.makeBug(title="generic")51 self.bug_one = self.factory.makeBug(title="generic")
@@ -128,12 +142,46 @@
128 rendered_comment, self.expected_comment_html)142 rendered_comment, self.expected_comment_html)
129143
130144
145class TestBugAttachments(TestCaseWithFactory):
146
147 layer = LaunchpadFunctionalLayer
148
149 def test_attachments_query_counts_constant(self):
150 login(USER_EMAIL)
151 self.bug = self.factory.makeBug()
152 self.factory.makeBugAttachment(self.bug)
153 self.factory.makeBugAttachment(self.bug)
154 webservice = LaunchpadWebServiceCaller(
155 'launchpad-library', 'salgado-change-anything')
156 collector = QueryCollector()
157 collector.register()
158 self.addCleanup(collector.unregister)
159 url = '/bugs/%d/attachments' % self.bug.id
160 response = webservice.get(url)
161 self.assertThat(collector, HasQueryCount(LessThan(24)))
162 with_2_count = collector.count
163 self.failUnlessEqual(response.status, 200)
164 login(USER_EMAIL)
165 self.factory.makeBugAttachment(self.bug)
166 logout()
167 response = webservice.get(url)
168 # XXX: Permit the second call to be == or less, because storm
169 # caching bugs (such as) https://bugs.launchpad.net/storm/+bug/619017
170 # which can cause spurious queries. There was an EC2 failure landing
171 # with this set to strictly equal which I could not reproduce locally,
172 # and the ec2 test did not report the storm egg used, so could not
173 # confidently rule out some form of skew.
174 self.assertThat(collector, HasQueryCount(MatchesAny(
175 Equals(with_2_count),
176 LessThan(with_2_count))))
177
178
131class TestBugMessages(TestCaseWithFactory):179class TestBugMessages(TestCaseWithFactory):
132180
133 layer = DatabaseFunctionalLayer181 layer = DatabaseFunctionalLayer
134182
135 def setUp(self):183 def setUp(self):
136 super(TestBugMessages, self).setUp('test@canonical.com')184 super(TestBugMessages, self).setUp(USER_EMAIL)
137 self.bug = self.factory.makeBug()185 self.bug = self.factory.makeBug()
138 self.message1 = self.factory.makeMessage()186 self.message1 = self.factory.makeMessage()
139 self.message2 = self.factory.makeMessage(parent=self.message1)187 self.message2 = self.factory.makeMessage(parent=self.message1)
@@ -191,7 +239,7 @@
191 real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT239 real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
192 snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3240 snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
193 try:241 try:
194 login('foo.bar@canonical.com')242 login(ADMIN_EMAIL)
195 for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):243 for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
196 person = self.factory.makePerson()244 person = self.factory.makePerson()
197 bug.subscribe(person, person)245 bug.subscribe(person, person)
198246
=== modified file 'lib/lp/testing/sampledata.py'
--- lib/lp/testing/sampledata.py 2010-08-26 12:33:03 +0000
+++ lib/lp/testing/sampledata.py 2010-08-30 05:20:59 +0000
@@ -53,7 +53,7 @@
53VCS_IMPORTS_MEMBER_EMAIL = 'david.allouche@canonical.com'53VCS_IMPORTS_MEMBER_EMAIL = 'david.allouche@canonical.com'
54COMMERCIAL_ADMIN_EMAIL = 'commercial-member@canonical.com'54COMMERCIAL_ADMIN_EMAIL = 'commercial-member@canonical.com'
55ADMIN_EMAIL = 'foo.bar@canonical.com'55ADMIN_EMAIL = 'foo.bar@canonical.com'
56SAMPLE_PERSON_EMAIL = 'test@canonical.com'56SAMPLE_PERSON_EMAIL = USER_EMAIL
57# A user that is an admin of ubuntu-team, which has upload rights57# A user that is an admin of ubuntu-team, which has upload rights
58# to Ubuntu.58# to Ubuntu.
59UBUNTU_DEVELOPER_ADMIN_NAME = 'name16'59UBUNTU_DEVELOPER_ADMIN_NAME = 'name16'