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
=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py 2009-12-15 13:47:52 +0000
+++ lib/lp/bugs/model/bugwatch.py 2009-12-15 17:28:51 +0000
@@ -29,6 +29,7 @@
29from canonical.database.enumcol import EnumCol29from canonical.database.enumcol import EnumCol
30from canonical.database.sqlbase import SQLBase30from canonical.database.sqlbase import SQLBase
31from canonical.launchpad.database.message import Message31from canonical.launchpad.database.message import Message
32from canonical.launchpad.helpers import shortlist
32from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities33from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
33from canonical.launchpad.validators.email import valid_email34from canonical.launchpad.validators.email import valid_email
34from canonical.launchpad.webapp import urlappend, urlsplit35from canonical.launchpad.webapp import urlappend, urlsplit
@@ -80,7 +81,8 @@
80 @property81 @property
81 def bugtasks(self):82 def bugtasks(self):
82 tasks = Store.of(self).find(BugTask, BugTask.bugwatch == self.id)83 tasks = Store.of(self).find(BugTask, BugTask.bugwatch == self.id)
83 return list(tasks.order_by(Desc(BugTask.datecreated)))84 tasks = tasks.order_by(Desc(BugTask.datecreated))
85 return shortlist(tasks, 10, 100)
8486
85 @property87 @property
86 def title(self):88 def title(self):
8789
=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
--- lib/lp/bugs/tests/test_bugwatch.py 2009-06-25 00:40:31 +0000
+++ lib/lp/bugs/tests/test_bugwatch.py 2009-12-15 17:28:51 +0000
@@ -11,13 +11,15 @@
1111
12from zope.component import getUtility12from zope.component import getUtility
1313
14from canonical.launchpad.ftests import login, ANONYMOUS14from canonical.launchpad.webapp import urlsplit
15from canonical.testing import (
16 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
17
15from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet18from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
16from lp.bugs.interfaces.bugwatch import (19from lp.bugs.interfaces.bugwatch import (
17 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)20 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
18from lp.registry.interfaces.person import IPersonSet21from lp.registry.interfaces.person import IPersonSet
19from canonical.launchpad.webapp import urlsplit22from lp.testing import ANONYMOUS, TestCaseWithFactory, login
20from canonical.testing import LaunchpadFunctionalLayer
2123
2224
23class ExtractBugTrackerAndBugTestBase(unittest.TestCase):25class ExtractBugTrackerAndBugTestBase(unittest.TestCase):
@@ -309,6 +311,20 @@
309 bug_id = '12345'311 bug_id = '12345'
310312
311313
314class TestBugWatchBugTasks(TestCaseWithFactory):
315
316 layer = DatabaseFunctionalLayer
317
318 def setUp(self):
319 super(TestBugWatchBugTasks, self).setUp('test@canonical.com')
320 self.bug_watch = self.factory.makeBugWatch()
321
322 def test_bugtasks(self):
323 # BugWatch.bugtasks is always a list.
324 self.assertIsInstance(
325 self.bug_watch.bugtasks, list)
326
327
312def test_suite():328def test_suite():
313 suite = unittest.TestSuite()329 suite = unittest.TestSuite()
314 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))330 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))
@@ -331,6 +347,7 @@
331 PHPProjectBugTrackerExtractBugTrackerAndBugTest))347 PHPProjectBugTrackerExtractBugTrackerAndBugTest))
332 suite.addTest(unittest.makeSuite(348 suite.addTest(unittest.makeSuite(
333 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))349 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))
350 suite.addTest(unittest.makeSuite(TestBugWatchBugTasks))
334 return suite351 return suite
335352
336353

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/browser/bugwatch.py'
--- lib/lp/bugs/browser/bugwatch.py 2009-09-03 15:47:41 +0000
+++ lib/lp/bugs/browser/bugwatch.py 2009-12-15 17:34:15 +0000
@@ -19,7 +19,6 @@
19from lp.bugs.browser.bugcomment import (19from lp.bugs.browser.bugcomment import (
20 should_display_remote_comments)20 should_display_remote_comments)
21from canonical.launchpad.fields import URIField21from canonical.launchpad.fields import URIField
22from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
23from canonical.launchpad.webapp.interfaces import ILaunchBag22from canonical.launchpad.webapp.interfaces import ILaunchBag
24from lp.bugs.interfaces.bug import IBugWatch23from lp.bugs.interfaces.bug import IBugWatch
25from lp.bugs.interfaces.bugwatch import (24from lp.bugs.interfaces.bugwatch import (
@@ -54,7 +53,6 @@
54 team, no comments will be returned.53 team, no comments will be returned.
55 """54 """
56 user = getUtility(ILaunchBag).user55 user = getUtility(ILaunchBag).user
57 lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
58 if not should_display_remote_comments(user):56 if not should_display_remote_comments(user):
59 return []57 return []
6058
@@ -121,7 +119,7 @@
121119
122 def bugWatchIsUnlinked(self, action):120 def bugWatchIsUnlinked(self, action):
123 """Return whether the bug watch is unlinked."""121 """Return whether the bug watch is unlinked."""
124 return self.context.bugtasks.count() == 0122 return len(self.context.bugtasks) == 0
125123
126 @action('Delete Bug Watch', name='delete', condition=bugWatchIsUnlinked)124 @action('Delete Bug Watch', name='delete', condition=bugWatchIsUnlinked)
127 def delete_action(self, action, data):125 def delete_action(self, action, data):
128126
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2009-10-15 04:00:34 +0000
+++ lib/lp/bugs/model/bugtask.py 2009-12-15 17:34:15 +0000
@@ -950,7 +950,6 @@
950950
951 def getPackageComponent(self):951 def getPackageComponent(self):
952 """See `IBugTask`."""952 """See `IBugTask`."""
953 sourcepackage = None
954 if ISourcePackage.providedBy(self.target):953 if ISourcePackage.providedBy(self.target):
955 return self.target.latest_published_component954 return self.target.latest_published_component
956 if IDistributionSourcePackage.providedBy(self.target):955 if IDistributionSourcePackage.providedBy(self.target):
957956
=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py 2009-09-04 09:48:04 +0000
+++ lib/lp/bugs/model/bugwatch.py 2009-12-15 17:34:15 +0000
@@ -15,34 +15,36 @@
15from zope.component import getUtility15from zope.component import getUtility
1616
17# SQL imports17# SQL imports
18from sqlobject import (ForeignKey, StringCol, SQLObjectNotFound,18from sqlobject import ForeignKey, SQLObjectNotFound, StringCol
19 SQLMultipleJoin)
2019
21from storm.expr import Not20from storm.expr import Desc, Not
22from storm.store import Store21from storm.store import Store
2322
24from lazr.lifecycle.event import ObjectModifiedEvent23from lazr.lifecycle.event import ObjectModifiedEvent
25from lazr.lifecycle.snapshot import Snapshot24from lazr.lifecycle.snapshot import Snapshot
26from lazr.uri import find_uris_in_text25from lazr.uri import find_uris_in_text
2726
28from canonical.database.sqlbase import SQLBase
29from canonical.database.constants import UTC_NOW27from canonical.database.constants import UTC_NOW
30from canonical.database.datetimecol import UtcDateTimeCol28from canonical.database.datetimecol import UtcDateTimeCol
31from canonical.database.enumcol import EnumCol29from canonical.database.enumcol import EnumCol
3230from canonical.database.sqlbase import SQLBase
33from lp.bugs.model.bugmessage import BugMessage
34from lp.bugs.model.bugset import BugSetBase
35from canonical.launchpad.database.message import Message31from canonical.launchpad.database.message import Message
32from canonical.launchpad.helpers import shortlist
36from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities33from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
34from canonical.launchpad.validators.email import valid_email
35from canonical.launchpad.webapp import urlappend, urlsplit
37from canonical.launchpad.webapp.interfaces import NotFoundError36from canonical.launchpad.webapp.interfaces import NotFoundError
37
38from lp.bugs.interfaces.bug import IBugWatch38from lp.bugs.interfaces.bug import IBugWatch
39from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet39from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
40from lp.bugs.interfaces.bugwatch import (40from lp.bugs.interfaces.bugwatch import (
41 BugWatchErrorType, IBugWatchSet, NoBugTrackerFound,41 BugWatchErrorType, IBugWatchSet, NoBugTrackerFound,
42 UnrecognizedBugTrackerURL)42 UnrecognizedBugTrackerURL)
43from canonical.launchpad.validators.email import valid_email43from lp.bugs.model.bugmessage import BugMessage
44from lp.bugs.model.bugset import BugSetBase
45from lp.bugs.model.bugtask import BugTask
44from lp.registry.interfaces.person import validate_public_person46from lp.registry.interfaces.person import validate_public_person
45from canonical.launchpad.webapp import urlappend, urlsplit47
4648
47BUG_TRACKER_URL_FORMATS = {49BUG_TRACKER_URL_FORMATS = {
48 BugTrackerType.BUGZILLA: 'show_bug.cgi?id=%s',50 BugTrackerType.BUGZILLA: 'show_bug.cgi?id=%s',
@@ -76,9 +78,11 @@
76 dbName='owner', foreignKey='Person',78 dbName='owner', foreignKey='Person',
77 storm_validator=validate_public_person, notNull=True)79 storm_validator=validate_public_person, notNull=True)
7880
79 # useful joins81 @property
80 bugtasks = SQLMultipleJoin('BugTask', joinColumn='bugwatch',82 def bugtasks(self):
81 orderBy=['-datecreated'])83 tasks = Store.of(self).find(BugTask, BugTask.bugwatch == self.id)
84 tasks = tasks.order_by(Desc(BugTask.datecreated))
85 return shortlist(tasks, 10, 100)
8286
83 @property87 @property
84 def title(self):88 def title(self):
@@ -162,7 +166,7 @@
162166
163 def destroySelf(self):167 def destroySelf(self):
164 """See `IBugWatch`."""168 """See `IBugWatch`."""
165 assert self.bugtasks.count() == 0, "Can't delete linked bug watches"169 assert len(self.bugtasks) == 0, "Can't delete linked bug watches"
166 SQLBase.destroySelf(self)170 SQLBase.destroySelf(self)
167171
168 def getLastErrorMessage(self):172 def getLastErrorMessage(self):
169173
=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
--- lib/lp/bugs/tests/test_bugwatch.py 2009-06-25 00:40:31 +0000
+++ lib/lp/bugs/tests/test_bugwatch.py 2009-12-15 17:34:15 +0000
@@ -11,13 +11,15 @@
1111
12from zope.component import getUtility12from zope.component import getUtility
1313
14from canonical.launchpad.ftests import login, ANONYMOUS14from canonical.launchpad.webapp import urlsplit
15from canonical.testing import (
16 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
17
15from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet18from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
16from lp.bugs.interfaces.bugwatch import (19from lp.bugs.interfaces.bugwatch import (
17 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)20 IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
18from lp.registry.interfaces.person import IPersonSet21from lp.registry.interfaces.person import IPersonSet
19from canonical.launchpad.webapp import urlsplit22from lp.testing import ANONYMOUS, TestCaseWithFactory, login
20from canonical.testing import LaunchpadFunctionalLayer
2123
2224
23class ExtractBugTrackerAndBugTestBase(unittest.TestCase):25class ExtractBugTrackerAndBugTestBase(unittest.TestCase):
@@ -309,6 +311,20 @@
309 bug_id = '12345'311 bug_id = '12345'
310312
311313
314class TestBugWatchBugTasks(TestCaseWithFactory):
315
316 layer = DatabaseFunctionalLayer
317
318 def setUp(self):
319 super(TestBugWatchBugTasks, self).setUp('test@canonical.com')
320 self.bug_watch = self.factory.makeBugWatch()
321
322 def test_bugtasks(self):
323 # BugWatch.bugtasks is always a list.
324 self.assertIsInstance(
325 self.bug_watch.bugtasks, list)
326
327
312def test_suite():328def test_suite():
313 suite = unittest.TestSuite()329 suite = unittest.TestSuite()
314 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))330 suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))
@@ -331,6 +347,7 @@
331 PHPProjectBugTrackerExtractBugTrackerAndBugTest))347 PHPProjectBugTrackerExtractBugTrackerAndBugTest))
332 suite.addTest(unittest.makeSuite(348 suite.addTest(unittest.makeSuite(
333 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))349 GoogleCodeBugTrackerExtractBugTrackerAndBugTest))
350 suite.addTest(unittest.makeSuite(TestBugWatchBugTasks))
334 return suite351 return suite
335352
336353