Merge lp:~bac/launchpad/bug-974617 into lp:launchpad

Proposed by Brad Crittenden on 2012-04-12
Status: Merged
Approved by: Brad Crittenden on 2012-04-12
Approved revision: no longer in the source branch.
Merged at revision: 15086
Proposed branch: lp:~bac/launchpad/bug-974617
Merge into: lp:launchpad
Diff against target: 59 lines (+26/-3)
1 file modified
lib/lp/services/webapp/tests/test_error.py (+26/-3)
To merge this branch: bzr merge lp:~bac/launchpad/bug-974617
Reviewer Review Type Date Requested Status
j.c.sackett (community) 2012-04-12 Approve on 2012-04-12
Richard Harding (community) code* 2012-04-12 Approve on 2012-04-12
Review via email: mp+101797@code.launchpad.net

Commit message

After restarting the pgbouncer in a test, retry a few times to give it a chance to come up.

Description of the change

Parallel testing exposed a problem where bouncer.start() was returning
but the bouncer service was not actually up.

This fix is gentler, giving the bouncer more time to tidy up and get started.

To post a comment you must log in.
Richard Harding (rharding) wrote :

Bac, looks like a fun issue. Couple of notes:

- What about having the retry count setup as a constant. DB_RETRIES or something in case it needs to be tweaked/adjusted.
- I'm not sure if this exception gets raised to a user or log, but it'd probably be helpful if it has some context to it. Maybe raising an EnvironmentError with a message that it failed to come up after X attempts?

review: Approve (code*)
j.c.sackett (jcsackett) wrote :

This looks good; I agree with Rick's request that the count be setup as a constant rather than a magic number in the iteration, but that's pretty minor.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/services/webapp/tests/test_error.py'
--- lib/lp/services/webapp/tests/test_error.py 2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/tests/test_error.py 2012-04-12 19:31:20 +0000
@@ -3,13 +3,15 @@
33
4"""Test error views."""4"""Test error views."""
55
6import urllib2
76
7import httplib
8from storm.exceptions import (8from storm.exceptions import (
9 DisconnectionError,9 DisconnectionError,
10 OperationalError,10 OperationalError,
11 )11 )
12import time
12import transaction13import transaction
14import urllib2
1315
14from lp.services.webapp.error import (16from lp.services.webapp.error import (
15 DisconnectionErrorView,17 DisconnectionErrorView,
@@ -26,6 +28,10 @@
26from lp.testing.matchers import Contains28from lp.testing.matchers import Contains
2729
2830
31class TimeoutException(Exception):
32 pass
33
34
29class TestSystemErrorView(TestCase):35class TestSystemErrorView(TestCase):
3036
31 layer = LaunchpadFunctionalLayer37 layer = LaunchpadFunctionalLayer
@@ -112,8 +118,25 @@
112 # We keep seeing the correct exception on subsequent requests.118 # We keep seeing the correct exception on subsequent requests.
113 self.assertEqual(503, self.getHTTPError(url).code)119 self.assertEqual(503, self.getHTTPError(url).code)
114 # When the database is available again, requests succeed.120 # When the database is available again, requests succeed.
115 bouncer.start()121 #bouncer.start()
116 urllib2.urlopen(url)122 # bouncer.start() can sometimes return before the service is actually
123 # available for use. To be defensive, let's retry a few times. See
124 # bug 974617.
125 retries = 5
126 for i in xrange(retries):
127 try:
128 urllib2.urlopen(url)
129 except urllib2.HTTPError as e:
130 if e.code != httplib.SERVICE_UNAVAILABLE:
131 raise
132 else:
133 break
134 time.sleep(0.5)
135 else:
136 raise TimeoutException(
137 "bouncer did not come up after {} attempts.".format(retries))
138
139
117140
118 def test_operationalerror_view(self):141 def test_operationalerror_view(self):
119 request = LaunchpadTestRequest()142 request = LaunchpadTestRequest()