Merge lp:~greglange/swift/lp682749 into lp:~hudson-openstack/swift/trunk
- lp682749
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | clayg |
Approved revision: | 136 |
Merged at revision: | 142 |
Proposed branch: | lp:~greglange/swift/lp682749 |
Merge into: | lp:~hudson-openstack/swift/trunk |
Diff against target: |
346 lines (+172/-67) 2 files modified
swift/proxy/server.py (+26/-20) test/unit/proxy/test_server.py (+146/-47) |
To merge this branch: | bzr merge lp:~greglange/swift/lp682749 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
clayg | Approve | ||
gholt (community) | Approve | ||
Review via email:
|
Commit message
Description of the change
Improved caching of results in the proxy server's account_info() and container_info() methods.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
clayg (clay-gerrard) wrote : | # |
on 507 shouldn't we attempts_left += 1 in case of handoffs?
Other than that I'm good with this too.
I really like these tests, nice work glange.
Random thought of the day, this might be wonky, but I think it be kinda cool if in check_account_
Cause then instead of calling:
self.check_
you would get to say:
self.check_
:)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Greg Lange (greglange) wrote : | # |
I think we should merge this without the 507 thing or the unit test changes (I like those how they are anyway = ).
If the 507 thing needs to be done, we should do that in a separate bug.
That's outside of what this bug is about.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
clayg (clay-gerrard) wrote : | # |
The 507 thing probably doesn't need to be done - writes use handoffs, reads generally shouldn't. Good call.
Preview Diff
1 | === modified file 'swift/proxy/server.py' | |||
2 | --- swift/proxy/server.py 2010-11-30 23:00:12 +0000 | |||
3 | +++ swift/proxy/server.py 2010-12-01 00:06:28 +0000 | |||
4 | @@ -88,10 +88,11 @@ | |||
5 | 88 | return func(*a, **kw) | 88 | return func(*a, **kw) |
6 | 89 | return wrapped | 89 | return wrapped |
7 | 90 | 90 | ||
8 | 91 | def get_account_memcache_key(account): | ||
9 | 92 | return 'account/%s' % account | ||
10 | 91 | 93 | ||
11 | 92 | def get_container_memcache_key(account, container): | 94 | def get_container_memcache_key(account, container): |
14 | 93 | path = '/%s/%s' % (account, container) | 95 | return 'container/%s/%s' % (account, container) |
13 | 94 | return 'container%s' % path | ||
15 | 95 | 96 | ||
16 | 96 | 97 | ||
17 | 97 | class Controller(object): | 98 | class Controller(object): |
18 | @@ -176,13 +177,17 @@ | |||
19 | 176 | if it does not exist | 177 | if it does not exist |
20 | 177 | """ | 178 | """ |
21 | 178 | partition, nodes = self.app.account_ring.get_nodes(account) | 179 | partition, nodes = self.app.account_ring.get_nodes(account) |
22 | 179 | path = '/%s' % account | ||
23 | 180 | cache_key = 'account%s' % path | ||
24 | 181 | # 0 = no responses, 200 = found, 404 = not found, -1 = mixed responses | 180 | # 0 = no responses, 200 = found, 404 = not found, -1 = mixed responses |
27 | 182 | if self.app.memcache and self.app.memcache.get(cache_key) == 200: | 181 | if self.app.memcache: |
28 | 183 | return partition, nodes | 182 | cache_key = get_account_memcache_key(account) |
29 | 183 | result_code = self.app.memcache.get(cache_key) | ||
30 | 184 | if result_code == 200: | ||
31 | 185 | return partition, nodes | ||
32 | 186 | elif result_code == 404: | ||
33 | 187 | return None, None | ||
34 | 184 | result_code = 0 | 188 | result_code = 0 |
35 | 185 | attempts_left = self.app.account_ring.replica_count | 189 | attempts_left = self.app.account_ring.replica_count |
36 | 190 | path = '/%s' % account | ||
37 | 186 | headers = {'x-cf-trans-id': self.trans_id} | 191 | headers = {'x-cf-trans-id': self.trans_id} |
38 | 187 | for node in self.iter_nodes(partition, nodes, self.app.account_ring): | 192 | for node in self.iter_nodes(partition, nodes, self.app.account_ring): |
39 | 188 | if self.error_limited(node): | 193 | if self.error_limited(node): |
40 | @@ -213,16 +218,16 @@ | |||
41 | 213 | except: | 218 | except: |
42 | 214 | self.exception_occurred(node, 'Account', | 219 | self.exception_occurred(node, 'Account', |
43 | 215 | 'Trying to get account info for %s' % path) | 220 | 'Trying to get account info for %s' % path) |
49 | 216 | if result_code == 200: | 221 | if self.app.memcache and result_code in (200, 404): |
50 | 217 | cache_timeout = self.app.recheck_account_existence | 222 | if result_code == 200: |
51 | 218 | else: | 223 | cache_timeout = self.app.recheck_account_existence |
52 | 219 | cache_timeout = self.app.recheck_account_existence * 0.1 | 224 | else: |
53 | 220 | if self.app.memcache: | 225 | cache_timeout = self.app.recheck_account_existence * 0.1 |
54 | 221 | self.app.memcache.set(cache_key, result_code, | 226 | self.app.memcache.set(cache_key, result_code, |
55 | 222 | timeout=cache_timeout) | 227 | timeout=cache_timeout) |
56 | 223 | if result_code == 200: | 228 | if result_code == 200: |
57 | 224 | return partition, nodes | 229 | return partition, nodes |
59 | 225 | return (None, None) | 230 | return None, None |
60 | 226 | 231 | ||
61 | 227 | def container_info(self, account, container): | 232 | def container_info(self, account, container): |
62 | 228 | """ | 233 | """ |
63 | @@ -239,7 +244,6 @@ | |||
64 | 239 | partition, nodes = self.app.container_ring.get_nodes( | 244 | partition, nodes = self.app.container_ring.get_nodes( |
65 | 240 | account, container) | 245 | account, container) |
66 | 241 | path = '/%s/%s' % (account, container) | 246 | path = '/%s/%s' % (account, container) |
67 | 242 | cache_key = None | ||
68 | 243 | if self.app.memcache: | 247 | if self.app.memcache: |
69 | 244 | cache_key = get_container_memcache_key(account, container) | 248 | cache_key = get_container_memcache_key(account, container) |
70 | 245 | cache_value = self.app.memcache.get(cache_key) | 249 | cache_value = self.app.memcache.get(cache_key) |
71 | @@ -249,8 +253,10 @@ | |||
72 | 249 | write_acl = cache_value['write_acl'] | 253 | write_acl = cache_value['write_acl'] |
73 | 250 | if status == 200: | 254 | if status == 200: |
74 | 251 | return partition, nodes, read_acl, write_acl | 255 | return partition, nodes, read_acl, write_acl |
75 | 256 | elif status == 404: | ||
76 | 257 | return None, None, None, None | ||
77 | 252 | if not self.account_info(account)[1]: | 258 | if not self.account_info(account)[1]: |
79 | 253 | return (None, None, None, None) | 259 | return None, None, None, None |
80 | 254 | result_code = 0 | 260 | result_code = 0 |
81 | 255 | read_acl = None | 261 | read_acl = None |
82 | 256 | write_acl = None | 262 | write_acl = None |
83 | @@ -290,11 +296,11 @@ | |||
84 | 290 | except: | 296 | except: |
85 | 291 | self.exception_occurred(node, 'Container', | 297 | self.exception_occurred(node, 'Container', |
86 | 292 | 'Trying to get container info for %s' % path) | 298 | 'Trying to get container info for %s' % path) |
92 | 293 | if result_code == 200: | 299 | if self.app.memcache and result_code in (200, 404): |
93 | 294 | cache_timeout = self.app.recheck_container_existence | 300 | if result_code == 200: |
94 | 295 | else: | 301 | cache_timeout = self.app.recheck_container_existence |
95 | 296 | cache_timeout = self.app.recheck_container_existence * 0.1 | 302 | else: |
96 | 297 | if cache_key and self.app.memcache: | 303 | cache_timeout = self.app.recheck_container_existence * 0.1 |
97 | 298 | self.app.memcache.set(cache_key, | 304 | self.app.memcache.set(cache_key, |
98 | 299 | {'status': result_code, | 305 | {'status': result_code, |
99 | 300 | 'read_acl': read_acl, | 306 | 'read_acl': read_acl, |
100 | @@ -303,7 +309,7 @@ | |||
101 | 303 | timeout=cache_timeout) | 309 | timeout=cache_timeout) |
102 | 304 | if result_code == 200: | 310 | if result_code == 200: |
103 | 305 | return partition, nodes, read_acl, write_acl | 311 | return partition, nodes, read_acl, write_acl |
105 | 306 | return (None, None, None, None) | 312 | return None, None, None, None |
106 | 307 | 313 | ||
107 | 308 | def iter_nodes(self, partition, nodes, ring): | 314 | def iter_nodes(self, partition, nodes, ring): |
108 | 309 | """ | 315 | """ |
109 | 310 | 316 | ||
110 | === modified file 'test/unit/proxy/test_server.py' | |||
111 | --- test/unit/proxy/test_server.py 2010-11-30 16:01:05 +0000 | |||
112 | +++ test/unit/proxy/test_server.py 2010-12-01 00:06:28 +0000 | |||
113 | @@ -88,6 +88,8 @@ | |||
114 | 88 | pass | 88 | pass |
115 | 89 | if 'slow' in kwargs: | 89 | if 'slow' in kwargs: |
116 | 90 | headers['content-length'] = '4' | 90 | headers['content-length'] = '4' |
117 | 91 | if 'headers' in kwargs: | ||
118 | 92 | headers.update(kwargs['headers']) | ||
119 | 91 | return headers.items() | 93 | return headers.items() |
120 | 92 | 94 | ||
121 | 93 | def read(self, amt=None): | 95 | def read(self, amt=None): |
122 | @@ -164,6 +166,9 @@ | |||
123 | 164 | def get(self, key): | 166 | def get(self, key): |
124 | 165 | return self.store.get(key) | 167 | return self.store.get(key) |
125 | 166 | 168 | ||
126 | 169 | def keys(self): | ||
127 | 170 | return self.store.keys() | ||
128 | 171 | |||
129 | 167 | def set(self, key, value, timeout=0): | 172 | def set(self, key, value, timeout=0): |
130 | 168 | self.store[key] = value | 173 | self.store[key] = value |
131 | 169 | return True | 174 | return True |
132 | @@ -201,10 +206,12 @@ | |||
133 | 201 | @contextmanager | 206 | @contextmanager |
134 | 202 | def save_globals(): | 207 | def save_globals(): |
135 | 203 | orig_http_connect = getattr(proxy_server, 'http_connect', None) | 208 | orig_http_connect = getattr(proxy_server, 'http_connect', None) |
136 | 209 | orig_account_info = getattr(proxy_server.Controller, 'account_info', None) | ||
137 | 204 | try: | 210 | try: |
138 | 205 | yield True | 211 | yield True |
139 | 206 | finally: | 212 | finally: |
140 | 207 | proxy_server.http_connect = orig_http_connect | 213 | proxy_server.http_connect = orig_http_connect |
141 | 214 | proxy_server.Controller.account_info = orig_account_info | ||
142 | 208 | 215 | ||
143 | 209 | # tests | 216 | # tests |
144 | 210 | 217 | ||
145 | @@ -212,63 +219,155 @@ | |||
146 | 212 | 219 | ||
147 | 213 | def setUp(self): | 220 | def setUp(self): |
148 | 214 | self.account_ring = FakeRing() | 221 | self.account_ring = FakeRing() |
149 | 222 | self.container_ring = FakeRing() | ||
150 | 223 | self.memcache = FakeMemcache() | ||
151 | 215 | 224 | ||
154 | 216 | app = proxy_server.Application(None, FakeMemcache(), | 225 | app = proxy_server.Application(None, self.memcache, |
155 | 217 | account_ring=self.account_ring, container_ring=FakeRing(), | 226 | account_ring=self.account_ring, |
156 | 227 | container_ring=self.container_ring, | ||
157 | 218 | object_ring=FakeRing()) | 228 | object_ring=FakeRing()) |
158 | 219 | self.controller = proxy_server.Controller(app) | 229 | self.controller = proxy_server.Controller(app) |
159 | 220 | 230 | ||
162 | 221 | def check_account_info_return(self, account, partition, nodes): | 231 | self.account = 'some_account' |
163 | 222 | p, n = self.account_ring.get_nodes(account) | 232 | self.container = 'some_container' |
164 | 233 | self.read_acl = 'read_acl' | ||
165 | 234 | self.write_acl = 'write_acl' | ||
166 | 235 | |||
167 | 236 | def check_account_info_return(self, partition, nodes, is_none=False): | ||
168 | 237 | if is_none: | ||
169 | 238 | p, n = None, None | ||
170 | 239 | else: | ||
171 | 240 | p, n = self.account_ring.get_nodes(self.account) | ||
172 | 223 | self.assertEqual(p, partition) | 241 | self.assertEqual(p, partition) |
173 | 224 | self.assertEqual(n, nodes) | 242 | self.assertEqual(n, nodes) |
174 | 225 | 243 | ||
178 | 226 | def test_account_info_404_200(self): | 244 | # tests if 200 is cached and used |
179 | 227 | account = 'test_account_info_404_200' | 245 | def test_account_info_200(self): |
177 | 228 | |||
180 | 229 | with save_globals(): | 246 | with save_globals(): |
181 | 230 | proxy_server.http_connect = fake_http_connect(404, 404, 404) | ||
182 | 231 | partition, nodes = self.controller.account_info(account) | ||
183 | 232 | self.assertEqual(partition, None) | ||
184 | 233 | self.assertEqual(nodes, None) | ||
185 | 234 | |||
186 | 235 | proxy_server.http_connect = fake_http_connect(200) | 247 | proxy_server.http_connect = fake_http_connect(200) |
190 | 236 | partition, nodes = self.controller.account_info(account) | 248 | partition, nodes = self.controller.account_info(self.account) |
191 | 237 | self.check_account_info_return(account, partition, nodes) | 249 | self.check_account_info_return(partition, nodes) |
192 | 238 | 250 | ||
193 | 251 | cache_key = proxy_server.get_account_memcache_key(self.account) | ||
194 | 252 | self.assertEquals(200, self.memcache.get(cache_key)) | ||
195 | 253 | |||
196 | 254 | proxy_server.http_connect = fake_http_connect() | ||
197 | 255 | partition, nodes = self.controller.account_info(self.account) | ||
198 | 256 | self.check_account_info_return(partition, nodes) | ||
199 | 257 | |||
200 | 258 | # tests if 404 is cached and used | ||
201 | 239 | def test_account_info_404(self): | 259 | def test_account_info_404(self): |
234 | 240 | account = 'test_account_info_404' | 260 | with save_globals(): |
235 | 241 | 261 | proxy_server.http_connect = fake_http_connect(404, 404, 404) | |
236 | 242 | with save_globals(): | 262 | partition, nodes = self.controller.account_info(self.account) |
237 | 243 | proxy_server.http_connect = fake_http_connect(404, 404, 404) | 263 | self.check_account_info_return(partition, nodes, True) |
238 | 244 | partition, nodes = self.controller.account_info(account) | 264 | |
239 | 245 | self.assertEqual(partition, None) | 265 | cache_key = proxy_server.get_account_memcache_key(self.account) |
240 | 246 | self.assertEqual(nodes, None) | 266 | self.assertEquals(404, self.memcache.get(cache_key)) |
241 | 247 | 267 | ||
242 | 248 | proxy_server.http_connect = fake_http_connect(404, 404, 404) | 268 | proxy_server.http_connect = fake_http_connect() |
243 | 249 | partition, nodes = self.controller.account_info(account) | 269 | partition, nodes = self.controller.account_info(self.account) |
244 | 250 | self.assertEqual(partition, None) | 270 | self.check_account_info_return(partition, nodes, True) |
245 | 251 | self.assertEqual(nodes, None) | 271 | |
246 | 252 | 272 | # tests if some http status codes are not cached | |
247 | 253 | def test_account_info_200(self): | 273 | def test_account_info_no_cache(self): |
248 | 254 | account = 'test_account_info_200' | 274 | def test(*status_list): |
249 | 255 | 275 | proxy_server.http_connect = fake_http_connect(*status_list) | |
250 | 256 | with save_globals(): | 276 | partition, nodes = self.controller.account_info(self.account) |
251 | 257 | proxy_server.http_connect = fake_http_connect(200) | 277 | self.assertEqual(len(self.memcache.keys()), 0) |
252 | 258 | partition, nodes = self.controller.account_info(account) | 278 | self.check_account_info_return(partition, nodes, True) |
253 | 259 | self.check_account_info_return(account, partition, nodes) | 279 | |
254 | 260 | 280 | with save_globals(): | |
255 | 261 | def test_account_info_200_200(self): | 281 | test(503, 404, 404) |
256 | 262 | account = 'test_account_info_200_200' | 282 | test(404, 404, 503) |
257 | 263 | 283 | test(404, 507, 503) | |
258 | 264 | with save_globals(): | 284 | test(503, 503, 503) |
259 | 265 | proxy_server.http_connect = fake_http_connect(200) | 285 | |
260 | 266 | partition, nodes = self.controller.account_info(account) | 286 | def check_container_info_return(self, ret, is_none=False): |
261 | 267 | self.check_account_info_return(account, partition, nodes) | 287 | if is_none: |
262 | 268 | 288 | partition, nodes, read_acl, write_acl = None, None, None, None | |
263 | 269 | proxy_server.http_connect = fake_http_connect(200) | 289 | else: |
264 | 270 | partition, nodes = self.controller.account_info(account) | 290 | partition, nodes = self.container_ring.get_nodes(self.account, |
265 | 271 | self.check_account_info_return(account, partition, nodes) | 291 | self.container) |
266 | 292 | read_acl, write_acl = self.read_acl, self.write_acl | ||
267 | 293 | self.assertEqual(partition, ret[0]) | ||
268 | 294 | self.assertEqual(nodes, ret[1]) | ||
269 | 295 | self.assertEqual(read_acl, ret[2]) | ||
270 | 296 | self.assertEqual(write_acl, ret[3]) | ||
271 | 297 | |||
272 | 298 | def test_container_info_invalid_account(self): | ||
273 | 299 | def account_info(self, account): | ||
274 | 300 | return None, None | ||
275 | 301 | |||
276 | 302 | with save_globals(): | ||
277 | 303 | proxy_server.Controller.account_info = account_info | ||
278 | 304 | ret = self.controller.container_info(self.account, | ||
279 | 305 | self.container) | ||
280 | 306 | self.check_container_info_return(ret, True) | ||
281 | 307 | |||
282 | 308 | # tests if 200 is cached and used | ||
283 | 309 | def test_container_info_200(self): | ||
284 | 310 | def account_info(self, account): | ||
285 | 311 | return True, True | ||
286 | 312 | |||
287 | 313 | with save_globals(): | ||
288 | 314 | headers = {'x-container-read': self.read_acl, | ||
289 | 315 | 'x-container-write': self.write_acl} | ||
290 | 316 | proxy_server.Controller.account_info = account_info | ||
291 | 317 | proxy_server.http_connect = fake_http_connect(200, | ||
292 | 318 | headers=headers) | ||
293 | 319 | ret = self.controller.container_info(self.account, | ||
294 | 320 | self.container) | ||
295 | 321 | self.check_container_info_return(ret) | ||
296 | 322 | |||
297 | 323 | cache_key = proxy_server.get_container_memcache_key(self.account, | ||
298 | 324 | self.container) | ||
299 | 325 | cache_value = self.memcache.get(cache_key) | ||
300 | 326 | self.assertEquals(dict, type(cache_value)) | ||
301 | 327 | self.assertEquals(200, cache_value.get('status')) | ||
302 | 328 | |||
303 | 329 | proxy_server.http_connect = fake_http_connect() | ||
304 | 330 | ret = self.controller.container_info(self.account, | ||
305 | 331 | self.container) | ||
306 | 332 | self.check_container_info_return(ret) | ||
307 | 333 | |||
308 | 334 | # tests if 404 is cached and used | ||
309 | 335 | def test_container_info_404(self): | ||
310 | 336 | def account_info(self, account): | ||
311 | 337 | return True, True | ||
312 | 338 | |||
313 | 339 | with save_globals(): | ||
314 | 340 | proxy_server.Controller.account_info = account_info | ||
315 | 341 | proxy_server.http_connect = fake_http_connect(404, 404, 404) | ||
316 | 342 | ret = self.controller.container_info(self.account, | ||
317 | 343 | self.container) | ||
318 | 344 | self.check_container_info_return(ret, True) | ||
319 | 345 | |||
320 | 346 | cache_key = proxy_server.get_container_memcache_key(self.account, | ||
321 | 347 | self.container) | ||
322 | 348 | cache_value = self.memcache.get(cache_key) | ||
323 | 349 | self.assertEquals(dict, type(cache_value)) | ||
324 | 350 | self.assertEquals(404, cache_value.get('status')) | ||
325 | 351 | |||
326 | 352 | proxy_server.http_connect = fake_http_connect() | ||
327 | 353 | ret = self.controller.container_info(self.account, | ||
328 | 354 | self.container) | ||
329 | 355 | self.check_container_info_return(ret, True) | ||
330 | 356 | |||
331 | 357 | # tests if some http status codes are not cached | ||
332 | 358 | def test_container_info_no_cache(self): | ||
333 | 359 | def test(*status_list): | ||
334 | 360 | proxy_server.http_connect = fake_http_connect(*status_list) | ||
335 | 361 | ret = self.controller.container_info(self.account, | ||
336 | 362 | self.container) | ||
337 | 363 | self.assertEqual(len(self.memcache.keys()), 0) | ||
338 | 364 | self.check_container_info_return(ret, True) | ||
339 | 365 | |||
340 | 366 | with save_globals(): | ||
341 | 367 | test(503, 404, 404) | ||
342 | 368 | test(404, 404, 503) | ||
343 | 369 | test(404, 507, 503) | ||
344 | 370 | test(503, 503, 503) | ||
345 | 272 | 371 | ||
346 | 273 | class TestProxyServer(unittest.TestCase): | 372 | class TestProxyServer(unittest.TestCase): |
347 | 274 | 373 |
I'm good with this. I'll wait until Monday to mark it Approved in case anybody else has anything.