Merge lp:~blamar/nova/lp728587 into lp:~hudson-openstack/nova/trunk
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Rick Harris on 2011-03-18 | ||||||||
Approved revision: | 806 | ||||||||
Merged at revision: | 830 | ||||||||
Proposed branch: | lp:~blamar/nova/lp728587 | ||||||||
Merge into: | lp:~hudson-openstack/nova/trunk | ||||||||
Diff against target: |
1238 lines (+912/-169) 8 files modified
etc/api-paste.ini (+1/-1) nova/api/openstack/__init__.py (+6/-0) nova/api/openstack/faults.py (+39/-0) nova/api/openstack/limits.py (+358/-0) nova/tests/api/openstack/__init__.py (+1/-1) nova/tests/api/openstack/fakes.py (+5/-5) nova/tests/api/openstack/test_adminapi.py (+0/-1) nova/tests/api/openstack/test_limits.py (+502/-161) |
||||||||
To merge this branch: | bzr merge lp:~blamar/nova/lp728587 | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Rick Harris (community) | Approve on 2011-03-18 | ||
Paul Voccio (community) | 2011-03-16 | Approve on 2011-03-17 | |
Titan | 2011-03-16 | Pending | |
Mark Washenberger | 2011-03-16 | Pending | |
Review via email:
|
Commit message
Re-implementation (or just implementation in many cases) of Limits in the OpenStack API. Limits is now available through /limits and the concept of a limit has been extended to include arbitrary regex / http verb combinations along with correct XML/JSON serialization. Tests included.
Description of the change
The addition of a /limits resource was a larger job than I had first thought. The original rate-limiting middleware and supporting classes were being tested through `time.sleep` calls instead of actually unit-testing the logic.
I've replaced most of the classes with something that is more testable and much more compatible with the output we're generating for the 1.1 API.
Overall we should have much more test coverage than with the previous limits implementation.
- 800. By Brian Lamar on 2011-03-16
-
Removed VIM specific stuff and changed copyright from 2010 to 2011.
- 801. By Brian Lamar on 2011-03-16
-
Added tests back for RateLimitingMid
dleware which now throw correctly serialized
errors with correct error codes.Removed some error printing, and simplified some other parts of the code with
suggestions from teammates. - 802. By Brian Lamar on 2011-03-16
-
Added i18n to error message.
Jay Pipes (jaypipes) wrote : | # |
Jay Pipes (jaypipes) wrote : | # |
Also, if you don't select *any* reviewer, it defaults to nova-core, just an FYI...
Brian Lamar (blamar) wrote : | # |
Hey Jay, I was waiting to hear back from a teammate before submitting directly to nova-core. I'll add them now since I realize the deadline is coming up soonish! Thanks again.
- 803. By Brian Lamar on 2011-03-17
-
Merged trunk.
Paul Voccio (pvo) wrote : | # |
nova/api/
nova/api/
got a few pep8 nits to go over. Will look for the remerge.
- 804. By Brian Lamar on 2011-03-17
-
Fixed pep8 violation.
- 805. By Brian Lamar on 2011-03-17
-
Pep8 error, oddly specific to pep8 v0.5 < x > v0.6
Rick Harris (rconradharris) wrote : | # |
Great work, Brian.
Some femto-nits:
> 245 + self.remaining = math.floor((cap - water) / cap * val)
This might benefit from one more paren just to make it abundantly clear what's going on:
self.remaining = math.floor(((cap - water) / cap) * val)
> 901 + return sum(filter(lambda x: x != None, results))
Might be clearer as:
return sum(result for result in results if result)
As I understand it, the recommendation is to favor list-comprehensions rather than filter/map/reduce where possible.
Brian Lamar (blamar) wrote : | # |
Thanks Rick. I hate to see the day that people don't quite get order of operations, but it's a great femto-nit (I'd even accept it as a pico-nit!).
As for the sum() line, I'd like to say your solution is more clear...but it is at least more efficient. I must not have thought about that line too long because now that I look at it:
---
return sum(filter(lambda x: x != None, results))
is the same as
return sum(filter(None, results))
---
That being said I understand list comprehensions (or actually in this case I think it ends up being a generator comprehension, which I just learned about!) are preferred so I've gone ahead and made these changes.
Thanks a lot for the review!
- 806. By Brian Lamar on 2011-03-17
-
Better comment for fault. Improved readability of two small sections.
Hi Brian! Just FYI, make sure you select "nova-core" as the reviewer when you do a merge request. You can add additional reviewers with the "Request another review" link on the merge proposal page (and I encourage you to do so, like you've done!)
The reason to do this is because nova-core sends email notifications to the dozen or so core committers, hopefully prodding more reviews :)
cheers!
jay