)]}'
{"nova/db/api.py":[{"author":{"_account_id":1030,"name":"Chris Behrens","email":"cbehrens@codestud.com","username":"cbehrens"},"change_message_id":"0eb6b837f25a68c008b4bbbf3c031d343fad00b0","unresolved":false,"context_lines":[{"line_number":254,"context_line":"def floating_ip_count_by_project(context, project_id, session\u003dNone):"},{"line_number":255,"context_line":"    \"\"\"Count floating ips used by project.\"\"\""},{"line_number":256,"context_line":"    return IMPL.floating_ip_count_by_project(context, project_id,"},{"line_number":257,"context_line":"                                             session\u003dsession)"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"def floating_ip_deallocate(context, address):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAHX%2F%2F8NA%3D","line":257,"updated":"2012-05-14 17:51:02.000000000","message":"Did you mean to expose \u0027session\u0027 for the calls in this file?  I need to come back to this review later, so I didn\u0027t fully grep for the calls...  But I see references in comments you added in quota.py saying that we don\u0027t expose the session outside of the DB API... so I assume that policy hasn\u0027t changed and the session arguments here are a mistake.","commit_id":"9ee12ca5d04f7e35a149738ee3cc12d3ef52bf34"},{"author":{"_account_id":1030,"name":"Chris Behrens","email":"cbehrens@codestud.com","username":"cbehrens"},"change_message_id":"8bbdee87ed1fb2a326a1e725e6c7ea8473d8e03b","unresolved":false,"context_lines":[{"line_number":254,"context_line":"def floating_ip_count_by_project(context, project_id, session\u003dNone):"},{"line_number":255,"context_line":"    \"\"\"Count floating ips used by project.\"\"\""},{"line_number":256,"context_line":"    return IMPL.floating_ip_count_by_project(context, project_id,"},{"line_number":257,"context_line":"                                             session\u003dsession)"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"def floating_ip_deallocate(context, address):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAHX%2F%2F8HM%3D","line":257,"in_reply_to":"AAAAHX%2F%2F8Mw%3D","updated":"2012-05-14 20:27:52.000000000","message":"Ah, callbacks... makes sense!","commit_id":"9ee12ca5d04f7e35a149738ee3cc12d3ef52bf34"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"14795748bb1dd5ef48015f5336756094f86751ca","unresolved":false,"context_lines":[{"line_number":254,"context_line":"def floating_ip_count_by_project(context, project_id, session\u003dNone):"},{"line_number":255,"context_line":"    \"\"\"Count floating ips used by project.\"\"\""},{"line_number":256,"context_line":"    return IMPL.floating_ip_count_by_project(context, project_id,"},{"line_number":257,"context_line":"                                             session\u003dsession)"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"def floating_ip_deallocate(context, address):"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAHX%2F%2F8Mw%3D","line":257,"in_reply_to":"AAAAHX%2F%2F8NA%3D","updated":"2012-05-14 17:56:43.000000000","message":"Yes, I did mean to expose session here, but only as an opaque pass-through.  You\u0027ll see why this is as you keep reviewing, but here\u0027s the gist:\n\nQuota checks have to be done within a transaction.  That includes synchronizing current usages, i.e., counting how many instances are in use, etc.  To facilitate this counting, there is a callback declared on those types of quota resources; that means that the session has to be passed into them.\n\nI am treating the session variable as an opaque state variable; session is simply passed through from the DB API, through the callback, and back into the DB API.","commit_id":"9ee12ca5d04f7e35a149738ee3cc12d3ef52bf34"}],"nova/db/sqlalchemy/migrate_repo/versions/086_quota_usages_reservations.py":[{"author":{"_account_id":360,"name":"Dan Prince","email":"dprince@redhat.com","username":"dan-prince"},"change_message_id":"453e57dd75557d6ebacb4cf9b21853a2f37d3c9b","unresolved":false,"context_lines":[{"line_number":42,"context_line":"                          _warn_on_bytestring\u003dFalse)),"},{"line_number":43,"context_line":"            Column(\u0027in_use\u0027, Integer(), nullable\u003dFalse),"},{"line_number":44,"context_line":"            Column(\u0027reserved\u0027, Integer(), nullable\u003dFalse),"},{"line_number":45,"context_line":"            Column(\u0027until_refresh\u0027, Integer(), nullable\u003dFalse),"},{"line_number":46,"context_line":"            )"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAHH%2F%2F%2FMY%3D","line":45,"updated":"2012-04-26 17:43:21.000000000","message":"I\u0027d like to see us start adding explicit mysql args here:\n\n        mysql_engine\u003d\u0027InnoDB\u0027,\n        mysql_charset\u003d\u0027utf8\u0027\n\nIf we start doing this in our migration I think we\u0027ll be able to avoid the clunky \u0027ALTER TABLE\u0027 commands to switch the engine and charset to UTF8.","commit_id":"3bfcb1e14db329afee095d6f1c51c99de5ef013d"}],"nova/db/sqlalchemy/migrate_repo/versions/097_quota_usages_reservations.py":[{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":44,"context_line":"            Column(\u0027reserved\u0027, Integer(), nullable\u003dFalse),"},{"line_number":45,"context_line":"            Column(\u0027until_refresh\u0027, Integer(), nullable\u003dTrue),"},{"line_number":46,"context_line":"            mysql_engine\u003d\u0027InnoDB\u0027,"},{"line_number":47,"context_line":"            mysql_charset\u003d\u0027utf8\u0027,"},{"line_number":48,"context_line":"            )"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7rM%3D","line":47,"updated":"2012-05-15 18:05:21.000000000","message":"In the Essex migration this is commented out; should we apply this to all of the tables at once in a future migration?","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":44,"context_line":"            Column(\u0027reserved\u0027, Integer(), nullable\u003dFalse),"},{"line_number":45,"context_line":"            Column(\u0027until_refresh\u0027, Integer(), nullable\u003dTrue),"},{"line_number":46,"context_line":"            mysql_engine\u003d\u0027InnoDB\u0027,"},{"line_number":47,"context_line":"            mysql_charset\u003d\u0027utf8\u0027,"},{"line_number":48,"context_line":"            )"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7oM%3D","line":47,"in_reply_to":"AAAAHX%2F%2F7rM%3D","updated":"2012-05-15 18:42:28.000000000","message":"If I recall correctly, I was asked to add this in an earlier review.  And, doesn\u0027t one of the existing migrations already do this?","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"}],"nova/db/sqlalchemy/models.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":458,"context_line":"    __tablename__ \u003d \u0027quota_usages\u0027"},{"line_number":459,"context_line":"    id \u003d Column(Integer, primary_key\u003dTrue)"},{"line_number":460,"context_line":""},{"line_number":461,"context_line":"    project_id \u003d Column(String(255))"},{"line_number":462,"context_line":"    resource \u003d Column(String(255))"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"    in_use \u003d Column(Integer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2B4%3D","line":461,"updated":"2012-04-24 21:16:53.000000000","message":"Virtually all SELECT statements involving Quotas in the SQLAlchemy API calls involve a filter on project_id. It would be wise to add an index on that column now.","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c10d230b90cf3b9786b4394193aac70b992e495d","unresolved":false,"context_lines":[{"line_number":458,"context_line":"    __tablename__ \u003d \u0027quota_usages\u0027"},{"line_number":459,"context_line":"    id \u003d Column(Integer, primary_key\u003dTrue)"},{"line_number":460,"context_line":""},{"line_number":461,"context_line":"    project_id \u003d Column(String(255))"},{"line_number":462,"context_line":"    resource \u003d Column(String(255))"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"    in_use \u003d Column(Integer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FtU%3D","line":461,"in_reply_to":"AAAAHH%2F%2F%2F%2B4%3D","updated":"2012-04-25 17:20:13.000000000","message":"Done","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":458,"context_line":"    __tablename__ \u003d \u0027quota_usages\u0027"},{"line_number":459,"context_line":"    id \u003d Column(Integer, primary_key\u003dTrue)"},{"line_number":460,"context_line":""},{"line_number":461,"context_line":"    project_id \u003d Column(String(255))"},{"line_number":462,"context_line":"    resource \u003d Column(String(255))"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"    in_use \u003d Column(Integer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F0A%3D","line":461,"in_reply_to":"AAAAHH%2F%2F%2F%2B4%3D","updated":"2012-04-25 15:00:37.000000000","message":"Probably :)  I\u0027ll have to look for examples of adding indexes…","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":486,"context_line":"                              \u0027Reservation.usage_id \u003d\u003d QuotaUsage.id,\u0027"},{"line_number":487,"context_line":"                              \u0027Reservation.deleted \u003d\u003d False)\u0027)"},{"line_number":488,"context_line":""},{"line_number":489,"context_line":"    project_id \u003d Column(String(255))"},{"line_number":490,"context_line":"    resource \u003d Column(String(255))"},{"line_number":491,"context_line":""},{"line_number":492,"context_line":"    delta \u003d Column(Integer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2B0%3D","line":489,"updated":"2012-04-24 21:16:53.000000000","message":"Same comment as above about adding an index on project_id. It is a critical index considering the access patterns for quotas and reservations in the DB API.","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c10d230b90cf3b9786b4394193aac70b992e495d","unresolved":false,"context_lines":[{"line_number":486,"context_line":"                              \u0027Reservation.usage_id \u003d\u003d QuotaUsage.id,\u0027"},{"line_number":487,"context_line":"                              \u0027Reservation.deleted \u003d\u003d False)\u0027)"},{"line_number":488,"context_line":""},{"line_number":489,"context_line":"    project_id \u003d Column(String(255))"},{"line_number":490,"context_line":"    resource \u003d Column(String(255))"},{"line_number":491,"context_line":""},{"line_number":492,"context_line":"    delta \u003d Column(Integer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FtQ%3D","line":489,"in_reply_to":"AAAAHH%2F%2F%2F%2B0%3D","updated":"2012-04-25 17:20:13.000000000","message":"Done","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"}],"nova/exception.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":697,"context_line":""},{"line_number":698,"context_line":""},{"line_number":699,"context_line":"class InvalidQuotaValue(Invalid):"},{"line_number":700,"context_line":"    message \u003d _(\"Change would make usage less than 0 for resources %(unders)s\")"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":""},{"line_number":703,"context_line":"class QuotaNotFound(NotFound):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2Bw%3D","line":700,"updated":"2012-04-24 21:16:53.000000000","message":"This error message seems a little odd... what is the unders variable containing?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":697,"context_line":""},{"line_number":698,"context_line":""},{"line_number":699,"context_line":"class InvalidQuotaValue(Invalid):"},{"line_number":700,"context_line":"    message \u003d _(\"Change would make usage less than 0 for resources %(unders)s\")"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":""},{"line_number":703,"context_line":"class QuotaNotFound(NotFound):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2Fz4%3D","line":700,"in_reply_to":"AAAAHH%2F%2F%2F%2Bw%3D","updated":"2012-04-25 15:00:37.000000000","message":"The unders variable contains a sorted list of the names of the resources for which the in_use count in the quota_usages table would be made negative by the requested change.  Any ideas on how best to reword this message?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a1baae382d98514cc81c4d20929a154bcd41c2d9","unresolved":false,"context_lines":[{"line_number":697,"context_line":""},{"line_number":698,"context_line":""},{"line_number":699,"context_line":"class InvalidQuotaValue(Invalid):"},{"line_number":700,"context_line":"    message \u003d _(\"Change would make usage less than 0 for resources %(unders)s\")"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":""},{"line_number":703,"context_line":"class QuotaNotFound(NotFound):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FsM%3D","line":700,"in_reply_to":"AAAAHH%2F%2F%2Fz4%3D","updated":"2012-04-25 17:36:44.000000000","message":"Got it, thx. Maybe changing it to read:\n\n Change would make usage less than 0 for the following resources: %(unders)s\n\nMight be good, but not a biggie. :)","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"}],"nova/quota.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":250,"context_line":"    return FLAGS.quota_injected_file_path_bytes"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"class QuotaDriver(object):"},{"line_number":254,"context_line":"    \"\"\""},{"line_number":255,"context_line":"    Driver to perform necessary checks to enforce quotas and obtain"},{"line_number":256,"context_line":"    quota information.  The default driver utilizes the local"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2Bs%3D","line":253,"updated":"2012-04-24 21:16:53.000000000","message":"I would prefer to see this class named DbQuotaDriver (and the above quota_driver flag changed accordingly) to make it clear that the driver uses a DB to store quotas/reservations...","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c10d230b90cf3b9786b4394193aac70b992e495d","unresolved":false,"context_lines":[{"line_number":250,"context_line":"    return FLAGS.quota_injected_file_path_bytes"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"class QuotaDriver(object):"},{"line_number":254,"context_line":"    \"\"\""},{"line_number":255,"context_line":"    Driver to perform necessary checks to enforce quotas and obtain"},{"line_number":256,"context_line":"    quota information.  The default driver utilizes the local"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FtM%3D","line":253,"in_reply_to":"AAAAHH%2F%2F%2F%2Bs%3D","updated":"2012-04-25 17:20:13.000000000","message":"Done","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":250,"context_line":"    return FLAGS.quota_injected_file_path_bytes"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"class QuotaDriver(object):"},{"line_number":254,"context_line":"    \"\"\""},{"line_number":255,"context_line":"    Driver to perform necessary checks to enforce quotas and obtain"},{"line_number":256,"context_line":"    quota information.  The default driver utilizes the local"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2Fz0%3D","line":253,"in_reply_to":"AAAAHH%2F%2F%2F%2Bs%3D","updated":"2012-04-25 15:00:37.000000000","message":"OK, no problem with that…","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self.name \u003d name"},{"line_number":552,"context_line":"        self.flag \u003d flag"},{"line_number":553,"context_line":"        self.sync \u003d None"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def quota(self, driver, context, **kwargs):"},{"line_number":556,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2Bk%3D","line":553,"updated":"2012-04-24 21:16:53.000000000","message":"AFAIU, Resource.sync is a callable. Why not make it a method here instead of a None-valued property?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self.name \u003d name"},{"line_number":552,"context_line":"        self.flag \u003d flag"},{"line_number":553,"context_line":"        self.sync \u003d None"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def quota(self, driver, context, **kwargs):"},{"line_number":556,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FzU%3D","line":553,"in_reply_to":"AAAAHH%2F%2F%2F%2Bk%3D","updated":"2012-04-25 15:00:37.000000000","message":"I have two separate types of resources here.  One type of resource has its usage tracked by means of the database; this type has a synchronization routine (declared by means of a decorator), and examples include \"usages\" and \"ram\".  The second type of resource limits ephemeral counts, such as \"injected_files\" and \"injected_file_content_bytes\"; these resources aren\u0027t tracked in the database anywhere, and so there can be no synchronization routine.\n\nAccordingly, there are two different types of quota check.  Resources that have a declared synchronization routine have to be manipulated through the use of QUOTAS.reserve(), which consults the quota_usages table and potentially invokes the synchronization routine based on configurable freshness rules.  Resources that do not have a declared synchronization routine have to be manipulated through the use of QUOTAS.limit_check(), which doesn\u0027t have to be protected in a transaction since actual counts (rather than deltas) are passed in.\n\nI chose to use an externally identified synchronization routine rather than a simple method for several reasons:\n\n1) I can quickly and easily differentiate between the types of resources with a simple property check.\n\n2) I wanted the synchronization routine to be able to update multiple resources, partly because the existing interfaces for obtaining current counts are defined to return several values corresponding to different resources.\n\n3) I wanted resources to be trivially defined, in a style similar to cfg, rather than having to subclass the Resource class.\n\nIn retrospect, it might have been better to define two subclasses of Resource (SyncResource and NonSyncResource?) and make the sync routine be an argument to the constructor; that would also make the distinction between the two types of resources clearer, as well as dropping the need for orphan syncs which you noted below.  What do you think of that idea?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c10d230b90cf3b9786b4394193aac70b992e495d","unresolved":false,"context_lines":[{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self.name \u003d name"},{"line_number":552,"context_line":"        self.flag \u003d flag"},{"line_number":553,"context_line":"        self.sync \u003d None"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def quota(self, driver, context, **kwargs):"},{"line_number":556,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FtY%3D","line":553,"in_reply_to":"AAAAHH%2F%2F%2FzU%3D","updated":"2012-04-25 17:20:13.000000000","message":"Or maybe AbsoluteResource and RelativeResource?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a1baae382d98514cc81c4d20929a154bcd41c2d9","unresolved":false,"context_lines":[{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self.name \u003d name"},{"line_number":552,"context_line":"        self.flag \u003d flag"},{"line_number":553,"context_line":"        self.sync \u003d None"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def quota(self, driver, context, **kwargs):"},{"line_number":556,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2Frs%3D","line":553,"in_reply_to":"AAAAHH%2F%2F%2FzU%3D","updated":"2012-04-25 17:36:44.000000000","message":"Yeah, I would add the comments above as comments to the decorator below (so readers of the code will understand the design decisions better) and also, yes, I think getting rid of the orphan_syncs semantics and replacing with two subclasses would make the code easier to understand.\n\nI would recommend ReservableResource and AbsoluteResource :)","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":582,"context_line":""},{"line_number":583,"context_line":"        # Ditto for the quota class"},{"line_number":584,"context_line":"        quota_class \u003d (kwargs[\u0027quota_class\u0027] if \u0027quota_class\u0027 in kwargs"},{"line_number":585,"context_line":"                       else context.quota_class)"},{"line_number":586,"context_line":""},{"line_number":587,"context_line":"        # Look up the quota for the project"},{"line_number":588,"context_line":"        if project_id:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2Bg%3D","line":585,"updated":"2012-04-24 21:16:53.000000000","message":"this could also be done as:\n\n quota_class \u003d kwargs.get(\u0027quota_class\u0027, context.quota_class)\n\nno?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c10d230b90cf3b9786b4394193aac70b992e495d","unresolved":false,"context_lines":[{"line_number":582,"context_line":""},{"line_number":583,"context_line":"        # Ditto for the quota class"},{"line_number":584,"context_line":"        quota_class \u003d (kwargs[\u0027quota_class\u0027] if \u0027quota_class\u0027 in kwargs"},{"line_number":585,"context_line":"                       else context.quota_class)"},{"line_number":586,"context_line":""},{"line_number":587,"context_line":"        # Look up the quota for the project"},{"line_number":588,"context_line":"        if project_id:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FtI%3D","line":585,"in_reply_to":"AAAAHH%2F%2F%2F%2Bg%3D","updated":"2012-04-25 17:20:13.000000000","message":"Done","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":582,"context_line":""},{"line_number":583,"context_line":"        # Ditto for the quota class"},{"line_number":584,"context_line":"        quota_class \u003d (kwargs[\u0027quota_class\u0027] if \u0027quota_class\u0027 in kwargs"},{"line_number":585,"context_line":"                       else context.quota_class)"},{"line_number":586,"context_line":""},{"line_number":587,"context_line":"        # Look up the quota for the project"},{"line_number":588,"context_line":"        if project_id:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FzI%3D","line":585,"in_reply_to":"AAAAHH%2F%2F%2F%2Bg%3D","updated":"2012-04-25 15:00:37.000000000","message":"Yeah, it could, I just didn\u0027t think of it at the time :)  But to be honest, I\u0027m not 100% certain that this method is actually needed; I\u0027m planning on leaving it in until I\u0027ve finished the blueprint, at which I\u0027ll submit a final patch to remove the old quota handling and any unused code…","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":632,"context_line":"        # If we have an orphaned sync function, install it"},{"line_number":633,"context_line":"        if resource.name in self._orphaned_sync:"},{"line_number":634,"context_line":"            resource.sync \u003d self._orphaned_sync[resource.name]"},{"line_number":635,"context_line":"            del self._orphaned_sync[resource.name]"},{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    def register_resources(self, resources):"},{"line_number":638,"context_line":"        \"\"\"Register a list of resources.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2BU%3D","line":635,"updated":"2012-04-24 21:16:53.000000000","message":"I\u0027m a little fuzzy on the orphaned sync stuff in here... why not just have a default noop sync() method on the base Resource class?","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"ec5d7f90f19b4476fc06c9b6f425ece28e8b729a","unresolved":false,"context_lines":[{"line_number":632,"context_line":"        # If we have an orphaned sync function, install it"},{"line_number":633,"context_line":"        if resource.name in self._orphaned_sync:"},{"line_number":634,"context_line":"            resource.sync \u003d self._orphaned_sync[resource.name]"},{"line_number":635,"context_line":"            del self._orphaned_sync[resource.name]"},{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    def register_resources(self, resources):"},{"line_number":638,"context_line":"        \"\"\"Register a list of resources.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2FzA%3D","line":635,"in_reply_to":"AAAAHH%2F%2F%2F%2BU%3D","updated":"2012-04-25 15:00:37.000000000","message":"Well, I think I discussed most of the answer to your specific question above; I\u0027ll just take this opportunity to clarify that the \"orphaned\" sync routines essentially allows pre-declaring sync routines before the resource is even added to the QUOTAS object.","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a1baae382d98514cc81c4d20929a154bcd41c2d9","unresolved":false,"context_lines":[{"line_number":632,"context_line":"        # If we have an orphaned sync function, install it"},{"line_number":633,"context_line":"        if resource.name in self._orphaned_sync:"},{"line_number":634,"context_line":"            resource.sync \u003d self._orphaned_sync[resource.name]"},{"line_number":635,"context_line":"            del self._orphaned_sync[resource.name]"},{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    def register_resources(self, resources):"},{"line_number":638,"context_line":"        \"\"\"Register a list of resources.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2Fro%3D","line":635,"in_reply_to":"AAAAHH%2F%2F%2FzA%3D","updated":"2012-04-25 17:36:44.000000000","message":"Yup, answered above.","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8da9ded24f54dbe1209984e6e1737bb94d39d09c","unresolved":false,"context_lines":[{"line_number":678,"context_line":"                    self._orphaned_sync[resource] \u003d func"},{"line_number":679,"context_line":"            return func"},{"line_number":680,"context_line":""},{"line_number":681,"context_line":"        return decorator"},{"line_number":682,"context_line":""},{"line_number":683,"context_line":"    def get_by_project(self, context, project_id, resource):"},{"line_number":684,"context_line":"        \"\"\"Get a specific quota by project.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAHH%2F%2F%2F%2Bc%3D","line":681,"updated":"2012-04-24 21:16:53.000000000","message":"See above related comment/question about not using standard inheritance and subclassing instead of a custom decorator here... seems like it would have been a bit more readable if you had simply made a few subclasses of Resource that, for example, used db.instance_data_get_for_project or db.volume_data_get_for_project instead of doing the decorator thing here...","commit_id":"04da9eb0010c1a405339fc663a489fbec9daf050"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # Get the quotas for the appropriate class"},{"line_number":357,"context_line":"        if project_id \u003d\u003d context.project_id:"},{"line_number":358,"context_line":"            quota_class \u003d context.quota_class"},{"line_number":359,"context_line":"        if quota_class:"},{"line_number":360,"context_line":"            class_quotas \u003d db.quota_class_get_all_by_name(context, quota_class)"},{"line_number":361,"context_line":"        else:"},{"line_number":362,"context_line":"            class_quotas \u003d {}"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7sk%3D","line":359,"updated":"2012-05-15 18:05:21.000000000","message":"Not exactly sure what should happen if a `quota_class` and `project_id` are passed in.\n\nShould that `raise` since according to the comments above that\u0027s not expected, or should we use the passed in `quota_class` using the principle of least surprise?\n\nRight now it\u0027s just silently using the projects.quota_class which feels worse than either of the two above.","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # Get the quotas for the appropriate class"},{"line_number":357,"context_line":"        if project_id \u003d\u003d context.project_id:"},{"line_number":358,"context_line":"            quota_class \u003d context.quota_class"},{"line_number":359,"context_line":"        if quota_class:"},{"line_number":360,"context_line":"            class_quotas \u003d db.quota_class_get_all_by_name(context, quota_class)"},{"line_number":361,"context_line":"        else:"},{"line_number":362,"context_line":"            class_quotas \u003d {}"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7oA%3D","line":359,"in_reply_to":"AAAAHX%2F%2F7sk%3D","updated":"2012-05-15 18:42:28.000000000","message":"Here\u0027s my thinking on this: there are places where we could call get_project_quotas() on a project which is not the one in the current context; in this case, the quota_class parameter is provided to allow the caller to hint as to what the project\u0027s quota class is.  When the project *does* match the one in the context, however, then we want to ignore the quota_class parameter and use the actual quota class set in the context.","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":402,"context_line":"        if has_sync:"},{"line_number":403,"context_line":"            sync_filt \u003d lambda x: hasattr(x, \u0027sync\u0027)"},{"line_number":404,"context_line":"        else:"},{"line_number":405,"context_line":"            sync_filt \u003d lambda x: not hasattr(x, \u0027sync\u0027)"},{"line_number":406,"context_line":"        desired \u003d set(keys)"},{"line_number":407,"context_line":"        sub_resources \u003d dict((k, v) for k, v in resources.items()"},{"line_number":408,"context_line":"                             if k in desired and sync_filt(v))"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7sY%3D","line":405,"updated":"2012-05-15 18:05:21.000000000","message":"This behavior doesn\u0027t match the comment above. The code here is testing for the presence of the attribute, not its None-ness.","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":402,"context_line":"        if has_sync:"},{"line_number":403,"context_line":"            sync_filt \u003d lambda x: hasattr(x, \u0027sync\u0027)"},{"line_number":404,"context_line":"        else:"},{"line_number":405,"context_line":"            sync_filt \u003d lambda x: not hasattr(x, \u0027sync\u0027)"},{"line_number":406,"context_line":"        desired \u003d set(keys)"},{"line_number":407,"context_line":"        sub_resources \u003d dict((k, v) for k, v in resources.items()"},{"line_number":408,"context_line":"                             if k in desired and sync_filt(v))"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7nw%3D","line":405,"in_reply_to":"AAAAHX%2F%2F7sY%3D","updated":"2012-05-15 18:42:28.000000000","message":"Yeah, I split a single Resource class into a class hierarchy, and I guess I never got around to updating the comment.","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":713,"context_line":"class Quota(object):"},{"line_number":714,"context_line":"    \"\"\"Represent the set of recognized quotas.\"\"\""},{"line_number":715,"context_line":""},{"line_number":716,"context_line":"    def __init__(self, quota_driver\u003dNone):"},{"line_number":717,"context_line":"        \"\"\"Initialize a Quota object.\"\"\""},{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        if not quota_driver:"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7sI%3D","line":716,"updated":"2012-05-15 18:05:21.000000000","message":"MIght be more clear as `quota_driver_class`?","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":713,"context_line":"class Quota(object):"},{"line_number":714,"context_line":"    \"\"\"Represent the set of recognized quotas.\"\"\""},{"line_number":715,"context_line":""},{"line_number":716,"context_line":"    def __init__(self, quota_driver\u003dNone):"},{"line_number":717,"context_line":"        \"\"\"Initialize a Quota object.\"\"\""},{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        if not quota_driver:"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7ns%3D","line":716,"in_reply_to":"AAAAHX%2F%2F7sI%3D","updated":"2012-05-15 18:42:28.000000000","message":"Hmmm...yeah, probably...","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":886,"context_line":""},{"line_number":887,"context_line":"        try:"},{"line_number":888,"context_line":"            self._driver.commit(context, reservations)"},{"line_number":889,"context_line":"        except Exception:"},{"line_number":890,"context_line":"            # Log the exception, but otherwise ignore it"},{"line_number":891,"context_line":"            LOG.exception(_(\"Failed to commit reservations \""},{"line_number":892,"context_line":"                            \"%(reservations)s\") % locals())"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7sE%3D","line":889,"updated":"2012-05-15 18:05:21.000000000","message":"Do we really want to ignore all exceptions at this level?\n\nWould we want real exceptions to make it out of the quota-engine so calling code could make a decision on how it should fail?","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":886,"context_line":""},{"line_number":887,"context_line":"        try:"},{"line_number":888,"context_line":"            self._driver.commit(context, reservations)"},{"line_number":889,"context_line":"        except Exception:"},{"line_number":890,"context_line":"            # Log the exception, but otherwise ignore it"},{"line_number":891,"context_line":"            LOG.exception(_(\"Failed to commit reservations \""},{"line_number":892,"context_line":"                            \"%(reservations)s\") % locals())"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7nc%3D","line":889,"in_reply_to":"AAAAHX%2F%2F7sE%3D","updated":"2012-05-15 18:42:28.000000000","message":"I opted to ignore exceptions here because there is a recovery path: reservations expire after a period of time, and usages resynchronize.  QUOTAS.commit() is called after, say, an instance is fully allocated; I don\u0027t see the error as being big enough to require moving the instance to the ERROR state, or to require deleting the instance, since the synchronization stuff will come into play.  I did want to log the exception, however, as it would be an indication of suboptimal behavior that we could improve.","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"c500f90e1c7b8fe8952493d2e148490f249a1687","unresolved":false,"context_lines":[{"line_number":955,"context_line":"            context, project_id, session\u003dsession))"},{"line_number":956,"context_line":""},{"line_number":957,"context_line":""},{"line_number":958,"context_line":"QUOTAS \u003d Quota()"},{"line_number":959,"context_line":""},{"line_number":960,"context_line":""},{"line_number":961,"context_line":"resources \u003d ["}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7sA%3D","line":958,"updated":"2012-05-15 18:05:21.000000000","message":"This is nitpicking, so feel free to ignore, but `Quota` feels like a bit of a misnomer; that class is really representing something of a quota engine which applies a bunch of quota rules.\n\nI just mention because the change in plural-ness looked a bit odd:\n\nQUOTAS \u003d Quota()","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e2b96af084875c758852b7f73ba32625e21c797d","unresolved":false,"context_lines":[{"line_number":955,"context_line":"            context, project_id, session\u003dsession))"},{"line_number":956,"context_line":""},{"line_number":957,"context_line":""},{"line_number":958,"context_line":"QUOTAS \u003d Quota()"},{"line_number":959,"context_line":""},{"line_number":960,"context_line":""},{"line_number":961,"context_line":"resources \u003d ["}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAHX%2F%2F7nY%3D","line":958,"in_reply_to":"AAAAHX%2F%2F7sA%3D","updated":"2012-05-15 18:42:28.000000000","message":"Yeah, it\u0027s a tough thing to name :)","commit_id":"b7c3428e8f14094c54a351ec214421399db5e8c1"},{"author":{"_account_id":475,"name":"Rick Harris","email":"rick.harris@rackspace.com","username":"rconradharris"},"change_message_id":"ff5b3954a58a7a002c9cf29512109a89c95dc9ca","unresolved":false,"context_lines":[{"line_number":892,"context_line":"        try:"},{"line_number":893,"context_line":"            self._driver.commit(context, reservations)"},{"line_number":894,"context_line":"        except Exception:"},{"line_number":895,"context_line":"            # Log the exception, but otherwise ignore it"},{"line_number":896,"context_line":"            LOG.exception(_(\"Failed to commit reservations \""},{"line_number":897,"context_line":"                            \"%(reservations)s\") % locals())"},{"line_number":898,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAHX%2F%2F7co%3D","line":895,"updated":"2012-05-15 21:44:28.000000000","message":"A note explaining why it\u0027s safe to to ignore/log the exception would really useful here down-the-road.\n\nJust distilling your comments from the previous review would be perfect:\n\n\"\t\nKevin L. Mitchell\t\t1:42 PM\nI opted to ignore exceptions here because there is a recovery path: reservations expire after a period of time, and usages resynchronize. QUOTAS.commit() is called after, say, an instance is fully allocated; I don\u0027t see the error as being big enough to require moving the instance to the ERROR state, or to require deleting the instance, since the synchronization stuff will come into play. I did want to log the exception, however, as it would be an indication of suboptimal behavior that we could improve.\n\"","commit_id":"48a7470644eacf1ae9de0a429eb725ebed5380a1"}]}