Merge lp:~jkakar/storm/better-timeout-messages into lp:storm

Proposed by Jamu Kakar
Status: Merged
Approved by: Jamu Kakar
Approved revision: 371
Merged at revision: 370
Proposed branch: lp:~jkakar/storm/better-timeout-messages
Merge into: lp:storm
Diff against target: 135 lines (+42/-10)
5 files modified
storm/databases/postgres.py (+2/-1)
storm/exceptions.py (+4/-2)
storm/tracer.py (+20/-2)
tests/databases/postgres.py (+3/-2)
tests/tracer.py (+13/-3)
To merge this branch: bzr merge lp:~jkakar/storm/better-timeout-messages
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Robert Collins (community) Approve
Storm Developers Pending
Review via email: mp+32687@code.launchpad.net

Description of the change

This branch introduces the following changes:

- Adds a description when TimeoutError's are raised.

- Improves TimeoutTracer docstrings.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Looks great to me. I don't know the code well enough to spot any flaws :)

The TimeoutTracer docstring could be a little clearer in the first sentence.

Perhaps

"""Provide a timeout facility for connections to prevent rogue operations.

...
"""

review: Approve
371. By Jamu Kakar

- Improve docstring.

Revision history for this message
Jamu Kakar (jkakar) wrote :

Thanks Rob, I've updated the docstring to what you've suggested.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi, this looks fine and a clear improvement.

Is there any chance that changing the signature of TimeoutError.__init__ will break client code? It does seem unlikely, but I thought it was worth asking :-)

review: Approve
Revision history for this message
Jamu Kakar (jkakar) wrote :

Michael:

I wondered about the changes to TimeoutError.__init__ breaking
client code, too. The sense I have is that it would be fairly odd
for client code to use what is basically an internal Storm exception
type. I suspect that any non-Storm code using it is doing something
funky that we don't need to worry about supporting.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/databases/postgres.py'
2--- storm/databases/postgres.py 2009-07-07 02:06:59 +0000
3+++ storm/databases/postgres.py 2010-08-15 18:36:43 +0000
4@@ -377,4 +377,5 @@
5 # psycopg2.extensions.QueryCanceledError in the future.
6 if (isinstance(error, DatabaseError) and
7 "statement timeout" in str(error)):
8- raise TimeoutError(statement, params)
9+ raise TimeoutError("SQL server cancelled statement", statement,
10+ params)
11
12=== modified file 'storm/exceptions.py'
13--- storm/exceptions.py 2010-02-09 09:26:52 +0000
14+++ storm/exceptions.py 2010-08-15 18:36:43 +0000
15@@ -116,15 +116,17 @@
16 class DisconnectionError(OperationalError):
17 pass
18
19+
20 class TimeoutError(StormError):
21 """Raised by timeout tracers when remining time is over."""
22
23- def __init__(self, statement, params):
24+ def __init__(self, message, statement, params):
25+ self.message = message
26 self.statement = statement
27 self.params = params
28
29 def __str__(self):
30- return "%r, %r" % (self.statement, self.params)
31+ return "%r, %r, %r" % (self.message, self.statement, self.params)
32
33
34 class ConnectionBlockedError(StormError):
35
36=== modified file 'storm/tracer.py'
37--- storm/tracer.py 2009-02-04 01:53:02 +0000
38+++ storm/tracer.py 2010-08-15 18:36:43 +0000
39@@ -39,14 +39,32 @@
40
41
42 class TimeoutTracer(object):
43+ """Provide a timeout facility for connections to prevent rogue operations.
44+
45+ This tracer must be subclassed by backend-specific implementations that
46+ override C{connection_raw_execute_error}, C{set_statement_timeout} and
47+ C{get_remaining_time} methods.
48+ """
49
50 def __init__(self, granularity=5):
51 self.granularity = granularity
52
53- def connection_raw_execute(self, connection, raw_cursor, statement, params):
54+ def connection_raw_execute(self, connection, raw_cursor, statement,
55+ params):
56+ """Check timeout conditions before a statement is executed.
57+
58+ @param connection: The L{Connection} to the database.
59+ @param raw_cursor: A cursor object, specific to the backend being used.
60+ @param statement: The SQL statement to execute.
61+ @param params: The parameters to use with C{statement}.
62+ @raises TimeoutError: Raised if there isn't enough time left to
63+ execute C{statement}.
64+ """
65 remaining_time = self.get_remaining_time()
66 if remaining_time <= 0:
67- raise TimeoutError(statement, params)
68+ raise TimeoutError(
69+ "%d seconds remaining in time budget" % remaining_time,
70+ statement, params)
71
72 last_remaining_time = getattr(connection,
73 "_timeout_tracer_remaining_time", 0)
74
75=== modified file 'tests/databases/postgres.py'
76--- tests/databases/postgres.py 2009-07-29 15:40:35 +0000
77+++ tests/databases/postgres.py 2010-08-15 18:36:43 +0000
78@@ -567,7 +567,8 @@
79 try:
80 self.connection.execute(statement)
81 except TimeoutError, e:
82- self.assertEquals(e.statement, statement)
83- self.assertEquals(e.params, ())
84+ self.assertEqual("SQL server cancelled statement", e.message)
85+ self.assertEqual(statement, e.statement)
86+ self.assertEqual((), e.params)
87 else:
88 self.fail("TimeoutError not raised")
89
90=== modified file 'tests/tracer.py'
91--- tests/tracer.py 2009-02-04 01:53:02 +0000
92+++ tests/tracer.py 2010-08-15 18:36:43 +0000
93@@ -6,7 +6,6 @@
94 DebugTracer, TimeoutTracer, TimeoutError, _tracers)
95 from storm.expr import Variable
96
97-from tests.mocker import ARGS
98 from tests.helper import TestHelper
99
100
101@@ -204,6 +203,12 @@
102 class TimeoutTracerTest(TimeoutTracerTestBase):
103
104 def test_raise_not_implemented(self):
105+ """
106+ L{TimeoutTracer.connection_raw_execute_error},
107+ L{TimeoutTracer.set_statement_timeout} and
108+ L{TimeoutTracer.get_remaining_time} must all be implemented by
109+ backend-specific subclasses.
110+ """
111 self.assertRaises(NotImplementedError,
112 self.tracer.connection_raw_execute_error,
113 None, None, None, None, None)
114@@ -213,6 +218,10 @@
115 self.tracer.get_remaining_time)
116
117 def test_raise_timeout_error_when_no_remaining_time(self):
118+ """
119+ A L{TimeoutError} is raised if there isn't any time left when a
120+ statement is executed.
121+ """
122 tracer_mock = self.mocker.patch(self.tracer)
123 tracer_mock.get_remaining_time()
124 self.mocker.result(0)
125@@ -221,8 +230,9 @@
126 try:
127 self.execute()
128 except TimeoutError, e:
129- self.assertEquals(e.statement, self.statement)
130- self.assertEquals(e.params, self.params)
131+ self.assertEqual("0 seconds remaining in time budget", e.message)
132+ self.assertEqual(self.statement, e.statement)
133+ self.assertEqual(self.params, e.params)
134 else:
135 self.fail("TimeoutError not raised")
136

Subscribers

People subscribed via source and target branches

to status/vote changes: