Launchpad itself

Merge lp:~allenap/launchpad/bugwatch-bugtasks-as-a-list into lp:launchpad

  1. bugwatch-bugtasks-as-a-list
  2. Merge into devel
Proposed by Gavin Panella
Status: Merged
Approved by: Brad Crittenden
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~allenap/launchpad/bugwatch-bugtasks-as-a-list
Merge into: lp:launchpad
Diff against target: 164 lines (+38/-20)
4 files modified
lib/lp/bugs/browser/bugwatch.py (+1/-3)
lib/lp/bugs/model/bugtask.py (+0/-1)
lib/lp/bugs/model/bugwatch.py (+17/-13)
lib/lp/bugs/tests/test_bugwatch.py (+20/-3)
To merge this branch: bzr merge lp:~allenap/launchpad/bugwatch-bugtasks-as-a-list
Reviewer Review Type Date Requested Status
Данило Шеган (community) release-critical Approve
Brad Crittenden (community) code Approve
Review via email: mp+16200@code.launchpad.net

Commit message

Make BugWatch.bugtasks a list instead of a result set. This may help us narrow down the causes behind the 2009-12-14 outage.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Change BugWatch.bugtasks into a property that returns a list, instead of being a SQLMultipleJoin which returns a result set. This relates to:

  https://wiki.canonical.com/IncidentReports/2009-12-14-Launchpad-Appserver-Outage

The long-lived query in the paste linked to from the report seems to have come from this bugtasks property. By changing it to immediately suck the results back from the DB we (gmb and I) hope to try and isolate the real issue, or at least narrow it down.

Revision history for this message
Brad Crittenden (bac) wrote :

Gavin your code changes look good. I'm glad to see the conversion to storm and I hope the change helps to isolate the real problem.

review: Approve (code)
Revision history for this message
Gavin Panella (allenap) wrote :

<bac> so your code changes look good and innocent enough
<bac> can you explain, slowly and using short words as if talking to a child, how this will help? :)
<allenap> The query that seems to hang around a long time in the db is the one - afaict - that is fulfilling BugWatch.bugtasks.
<allenap> That's iterated over in BugWatch.updateStatus().
<allenap> My hunch is that something is happening in one or more of those iterations that is causing the results from the database to not be pulled.
<allenap> I'm hoping that by sucking the results in immediately we can narrow down the issue. Maybe.
<bac> ok. i just don't understand why this query would now be causing problems. we use similar queries everywhere.
<bac> like i said, the changes are innocuous and i like the fact they have been stormified so i'm ok with trying the change
<bac> you may want to update the MP with more information if you seek an RC
<allenap> I don't think this query is a problem. I think it's that nothing is sucking the results back, so the database keeps it around as an in progress query.

Revision history for this message
Gavin Panella (allenap) wrote :

I think this branch is worthy of release-critical because we need it to be in production to help us in narrow down issues in the checkwatches cron job; edge is not enough.

Revision history for this message
Данило Шеган (danilo) wrote :

Looks good, just use the shortlist() instead of list() to prevent abuse and DoS vector.

review: Approve (release-critical)
Revision history for this message
Gavin Panella (allenap) wrote :

> Review: Approve release-critical
> Looks good, just use the shortlist() instead of list() to prevent abuse and DoS vector.

Thanks Danilo. Diff attached.

post-danilo.diff
1=== modified file 'lib/lp/bugs/model/bugwatch.py'
2--- lib/lp/bugs/model/bugwatch.py 2009-12-15 13:47:52 +0000
3+++ lib/lp/bugs/model/bugwatch.py 2009-12-15 17:28:51 +0000
4@@ -29,6 +29,7 @@
5 from canonical.database.enumcol import EnumCol
6 from canonical.database.sqlbase import SQLBase
7 from canonical.launchpad.database.message import Message
8+from canonical.launchpad.helpers import shortlist
9 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
10 from canonical.launchpad.validators.email import valid_email
11 from canonical.launchpad.webapp import urlappend, urlsplit
12@@ -80,7 +81,8 @@
13 @property
14 def bugtasks(self):
15 tasks = Store.of(self).find(BugTask, BugTask.bugwatch == self.id)
16- return list(tasks.order_by(Desc(BugTask.datecreated)))
17+ tasks = tasks.order_by(Desc(BugTask.datecreated))
18+ return shortlist(tasks, 10, 100)
19
20 @property
21 def title(self):
22
23=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
24--- lib/lp/bugs/tests/test_bugwatch.py 2009-06-25 00:40:31 +0000
25+++ lib/lp/bugs/tests/test_bugwatch.py 2009-12-15 17:28:51 +0000
26@@ -11,13 +11,15 @@
27
28 from zope.component import getUtility
29
30-from canonical.launchpad.ftests import login, ANONYMOUS
31+from canonical.launchpad.webapp import urlsplit
32+from canonical.testing import (
33+ DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
34+
35 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
36 from lp.bugs.interfaces.bugwatch import (
37 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
38 from lp.registry.interfaces.person import IPersonSet
39-from canonical.launchpad.webapp import urlsplit
40-from canonical.testing import LaunchpadFunctionalLayer
41+from lp.testing import ANONYMOUS, TestCaseWithFactory, login
42
43
44 class ExtractBugTrackerAndBugTestBase(unittest.TestCase):
45@@ -309,6 +311,20 @@
46 bug_id = '12345'
47
48
49+class TestBugWatchBugTasks(TestCaseWithFactory):
50+
51+ layer = DatabaseFunctionalLayer
52+
53+ def setUp(self):
54+ super(TestBugWatchBugTasks, self).setUp('test@canonical.com')
55+ self.bug_watch = self.factory.makeBugWatch()
56+
57+ def test_bugtasks(self):
58+ # BugWatch.bugtasks is always a list.
59+ self.assertIsInstance(
60+ self.bug_watch.bugtasks, list)
61+
62+
63 def test_suite():
64 suite = unittest.TestSuite()
65 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))
66@@ -331,6 +347,7 @@
67 PHPProjectBugTrackerExtractBugTrackerAndBugTest))
68 suite.addTest(unittest.makeSuite(
69 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))
70+ suite.addTest(unittest.makeSuite(TestBugWatchBugTasks))
71 return suite
72
73

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugwatch.py'
2--- lib/lp/bugs/browser/bugwatch.py 2009-09-03 15:47:41 +0000
3+++ lib/lp/bugs/browser/bugwatch.py 2009-12-15 17:34:15 +0000
4@@ -19,7 +19,6 @@
5 from lp.bugs.browser.bugcomment import (
6 should_display_remote_comments)
7 from canonical.launchpad.fields import URIField
8-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
9 from canonical.launchpad.webapp.interfaces import ILaunchBag
10 from lp.bugs.interfaces.bug import IBugWatch
11 from lp.bugs.interfaces.bugwatch import (
12@@ -54,7 +53,6 @@
13 team, no comments will be returned.
14 """
15 user = getUtility(ILaunchBag).user
16- lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
17 if not should_display_remote_comments(user):
18 return []
19
20@@ -121,7 +119,7 @@
21
22 def bugWatchIsUnlinked(self, action):
23 """Return whether the bug watch is unlinked."""
24- return self.context.bugtasks.count() == 0
25+ return len(self.context.bugtasks) == 0
26
27 @action('Delete Bug Watch', name='delete', condition=bugWatchIsUnlinked)
28 def delete_action(self, action, data):
29
30=== modified file 'lib/lp/bugs/model/bugtask.py'
31--- lib/lp/bugs/model/bugtask.py 2009-10-15 04:00:34 +0000
32+++ lib/lp/bugs/model/bugtask.py 2009-12-15 17:34:15 +0000
33@@ -950,7 +950,6 @@
34
35 def getPackageComponent(self):
36 """See `IBugTask`."""
37- sourcepackage = None
38 if ISourcePackage.providedBy(self.target):
39 return self.target.latest_published_component
40 if IDistributionSourcePackage.providedBy(self.target):
41
42=== modified file 'lib/lp/bugs/model/bugwatch.py'
43--- lib/lp/bugs/model/bugwatch.py 2009-09-04 09:48:04 +0000
44+++ lib/lp/bugs/model/bugwatch.py 2009-12-15 17:34:15 +0000
45@@ -15,34 +15,36 @@
46 from zope.component import getUtility
47
48 # SQL imports
49-from sqlobject import (ForeignKey, StringCol, SQLObjectNotFound,
50- SQLMultipleJoin)
51+from sqlobject import ForeignKey, SQLObjectNotFound, StringCol
52
53-from storm.expr import Not
54+from storm.expr import Desc, Not
55 from storm.store import Store
56
57 from lazr.lifecycle.event import ObjectModifiedEvent
58 from lazr.lifecycle.snapshot import Snapshot
59 from lazr.uri import find_uris_in_text
60
61-from canonical.database.sqlbase import SQLBase
62 from canonical.database.constants import UTC_NOW
63 from canonical.database.datetimecol import UtcDateTimeCol
64 from canonical.database.enumcol import EnumCol
65-
66-from lp.bugs.model.bugmessage import BugMessage
67-from lp.bugs.model.bugset import BugSetBase
68+from canonical.database.sqlbase import SQLBase
69 from canonical.launchpad.database.message import Message
70+from canonical.launchpad.helpers import shortlist
71 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
72+from canonical.launchpad.validators.email import valid_email
73+from canonical.launchpad.webapp import urlappend, urlsplit
74 from canonical.launchpad.webapp.interfaces import NotFoundError
75+
76 from lp.bugs.interfaces.bug import IBugWatch
77 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
78 from lp.bugs.interfaces.bugwatch import (
79 BugWatchErrorType, IBugWatchSet, NoBugTrackerFound,
80 UnrecognizedBugTrackerURL)
81-from canonical.launchpad.validators.email import valid_email
82+from lp.bugs.model.bugmessage import BugMessage
83+from lp.bugs.model.bugset import BugSetBase
84+from lp.bugs.model.bugtask import BugTask
85 from lp.registry.interfaces.person import validate_public_person
86-from canonical.launchpad.webapp import urlappend, urlsplit
87+
88
89 BUG_TRACKER_URL_FORMATS = {
90 BugTrackerType.BUGZILLA: 'show_bug.cgi?id=%s',
91@@ -76,9 +78,11 @@
92 dbName='owner', foreignKey='Person',
93 storm_validator=validate_public_person, notNull=True)
94
95- # useful joins
96- bugtasks = SQLMultipleJoin('BugTask', joinColumn='bugwatch',
97- orderBy=['-datecreated'])
98+ @property
99+ def bugtasks(self):
100+ tasks = Store.of(self).find(BugTask, BugTask.bugwatch == self.id)
101+ tasks = tasks.order_by(Desc(BugTask.datecreated))
102+ return shortlist(tasks, 10, 100)
103
104 @property
105 def title(self):
106@@ -162,7 +166,7 @@
107
108 def destroySelf(self):
109 """See `IBugWatch`."""
110- assert self.bugtasks.count() == 0, "Can't delete linked bug watches"
111+ assert len(self.bugtasks) == 0, "Can't delete linked bug watches"
112 SQLBase.destroySelf(self)
113
114 def getLastErrorMessage(self):
115
116=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
117--- lib/lp/bugs/tests/test_bugwatch.py 2009-06-25 00:40:31 +0000
118+++ lib/lp/bugs/tests/test_bugwatch.py 2009-12-15 17:34:15 +0000
119@@ -11,13 +11,15 @@
120
121 from zope.component import getUtility
122
123-from canonical.launchpad.ftests import login, ANONYMOUS
124+from canonical.launchpad.webapp import urlsplit
125+from canonical.testing import (
126+ DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
127+
128 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
129 from lp.bugs.interfaces.bugwatch import (
130 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
131 from lp.registry.interfaces.person import IPersonSet
132-from canonical.launchpad.webapp import urlsplit
133-from canonical.testing import LaunchpadFunctionalLayer
134+from lp.testing import ANONYMOUS, TestCaseWithFactory, login
135
136
137 class ExtractBugTrackerAndBugTestBase(unittest.TestCase):
138@@ -309,6 +311,20 @@
139 bug_id = '12345'
140
141
142+class TestBugWatchBugTasks(TestCaseWithFactory):
143+
144+ layer = DatabaseFunctionalLayer
145+
146+ def setUp(self):
147+ super(TestBugWatchBugTasks, self).setUp('test@canonical.com')
148+ self.bug_watch = self.factory.makeBugWatch()
149+
150+ def test_bugtasks(self):
151+ # BugWatch.bugtasks is always a list.
152+ self.assertIsInstance(
153+ self.bug_watch.bugtasks, list)
154+
155+
156 def test_suite():
157 suite = unittest.TestSuite()
158 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))
159@@ -331,6 +347,7 @@
160 PHPProjectBugTrackerExtractBugTrackerAndBugTest))
161 suite.addTest(unittest.makeSuite(
162 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))
163+ suite.addTest(unittest.makeSuite(TestBugWatchBugTasks))
164 return suite
165
166