)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"e632781c86cb456e7f5bf10d958c14694ede1293","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"9890ea9c_c1f32f3c","updated":"2021-10-14 12:41:48.000000000","message":"LGTM","commit_id":"155aef40dd53cfd59f5f985a8224f3e1d6ab0fbb"},{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"353e9c018745689d9994f677493bafb0def35968","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"38ff3fce_8d63e249","updated":"2021-10-14 12:42:13.000000000","message":"Please can you add a release note?","commit_id":"155aef40dd53cfd59f5f985a8224f3e1d6ab0fbb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f067baa03eb5ed6c22fe6f2684fb11e7c470e8e8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ab32e7ea_1bc7ee97","in_reply_to":"38ff3fce_8d63e249","updated":"2021-10-14 18:35:19.000000000","message":"Yes, can do. I will upload a new PS. Thank you for reviewing!","commit_id":"155aef40dd53cfd59f5f985a8224f3e1d6ab0fbb"}],"oslo_limit/limit.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"af6b891727562a9813d0caea5034df5f58daf2ae","unresolved":true,"context_lines":[{"line_number":239,"context_line":"        # {"},{"line_number":240,"context_line":"        #     resource_name: registered_limit,"},{"line_number":241,"context_line":"        #     ..."},{"line_number":242,"context_line":"        #     project_id: {resource_name: project_limit},"},{"line_number":243,"context_line":"        #     ..."},{"line_number":244,"context_line":"        # }"},{"line_number":245,"context_line":"        self.cache \u003d defaultdict(dict) if cache else None"}],"source_content_type":"text/x-python","patch_set":1,"id":"7d4b6a31_e08c579b","line":242,"updated":"2021-07-29 13:53:23.000000000","message":"This seems dangerous to me. Presumably there\u0027s not much someone can do to mess this up by choosing their project_id, but registered limit names could be a potential clash. Imagine if I\u0027m confused about what a registered limit is supposed to be, and I create one with the name being the uuid of one of my tenants. I might cause this code to fetch a limit when it\u0027s expecting to fetch a project {name:limit} dict.\n\nMaybe we could namespace the keys here to prevent that? Or maybe even better, just have a registered_limit cache and a project cache?","commit_id":"53312ccfc67da4091ea9a6c4e9f71d476400b0bb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"202a51d909786528ac18e216fb4fcdfeed96f551","unresolved":true,"context_lines":[{"line_number":239,"context_line":"        # {"},{"line_number":240,"context_line":"        #     resource_name: registered_limit,"},{"line_number":241,"context_line":"        #     ..."},{"line_number":242,"context_line":"        #     project_id: {resource_name: project_limit},"},{"line_number":243,"context_line":"        #     ..."},{"line_number":244,"context_line":"        # }"},{"line_number":245,"context_line":"        self.cache \u003d defaultdict(dict) if cache else None"}],"source_content_type":"text/x-python","patch_set":1,"id":"8028f4d7_1bc76149","line":242,"in_reply_to":"7d4b6a31_e08c579b","updated":"2021-07-29 17:26:57.000000000","message":"Hm, yeah, I didn\u0027t consider the possibility of collisions due to project_id as registered limit name. I think it\u0027d be simpler to just use separate caches (as opposed to namespacing).","commit_id":"53312ccfc67da4091ea9a6c4e9f71d476400b0bb"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"af6b891727562a9813d0caea5034df5f58daf2ae","unresolved":true,"context_lines":[{"line_number":322,"context_line":"                project_limit \u003d self._get_project_limit(project_id,"},{"line_number":323,"context_line":"                                                        resource_name)"},{"line_number":324,"context_line":"                if project_limit:"},{"line_number":325,"context_line":"                    self.cache[project_id][resource_name] \u003d project_limit"},{"line_number":326,"context_line":"        else:"},{"line_number":327,"context_line":"            project_limit \u003d self._get_project_limit(project_id, resource_name)"},{"line_number":328,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"c54c7fe9_4f26808d","line":325,"updated":"2021-07-29 13:53:23.000000000","message":"I think this would be a lot simpler if you always make the cache a dict, and only *store* the value if caching is enabled. Then you don\u0027t need so many conditionals. So:\n\n if project_id in cache:\n     return cache[project_id]\n\n limit \u003d get_actual_limit()\n\n if should_cache and limit:\n     cache[project_id] \u003d limit\n\n return limit","commit_id":"53312ccfc67da4091ea9a6c4e9f71d476400b0bb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"202a51d909786528ac18e216fb4fcdfeed96f551","unresolved":true,"context_lines":[{"line_number":322,"context_line":"                project_limit \u003d self._get_project_limit(project_id,"},{"line_number":323,"context_line":"                                                        resource_name)"},{"line_number":324,"context_line":"                if project_limit:"},{"line_number":325,"context_line":"                    self.cache[project_id][resource_name] \u003d project_limit"},{"line_number":326,"context_line":"        else:"},{"line_number":327,"context_line":"            project_limit \u003d self._get_project_limit(project_id, resource_name)"},{"line_number":328,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"64a7b03c_ac86613a","line":325,"in_reply_to":"c54c7fe9_4f26808d","updated":"2021-07-29 17:26:57.000000000","message":"That makes sense, thanks.","commit_id":"53312ccfc67da4091ea9a6c4e9f71d476400b0bb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"44ccfe764370b90aea275d94761d1adefb4def73","unresolved":true,"context_lines":[{"line_number":299,"context_line":"            except _LimitNotFound:"},{"line_number":300,"context_line":"                missing_limits.append(resource_name)"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        if len(missing_limits) \u003e 0:"},{"line_number":303,"context_line":"            over_limit_list \u003d [exception.OverLimitInfo(name, 0, 0, 0)"},{"line_number":304,"context_line":"                               for name in missing_limits]"},{"line_number":305,"context_line":"            raise exception.ProjectOverLimit(project_id, over_limit_list)"},{"line_number":306,"context_line":""},{"line_number":307,"context_line":"        return project_limits"},{"line_number":308,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"31fc528c_e63a5f24","line":305,"range":{"start_line":302,"start_character":8,"end_line":305,"end_character":73},"updated":"2021-07-29 21:02:59.000000000","message":"Unrelated to this patch, I find this really confusing personally. When there\u0027s no limit found for DISK_GB, for example, the message is:\n\n oslo_limit.exception.ProjectOverLimit: Project admin is over a limit for [Resource class:DISK_GB is over limit of 0 due to current usage 0 and delta 0]\n\nI had assumed this ^ meant that missing limits were not taken into consideration in the enforce method, but now I see it was known/intentional.","commit_id":"155aef40dd53cfd59f5f985a8224f3e1d6ab0fbb"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"87bb6982f078de5cf2fbf5c1acd63378c5713f22","unresolved":true,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"class Enforcer(object):"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    def __init__(self, usage_callback, cache\u003dTrue):"},{"line_number":60,"context_line":"        \"\"\"An object for checking usage against resource limits and requests."},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"        :param usage_callback: A callable function that accepts a project_id"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba880691_052f5add","line":59,"range":{"start_line":59,"start_character":37,"end_line":59,"end_character":39},"updated":"2022-01-10 11:52:03.000000000","message":"super nit: this seems like something that would really make sense as a kwarg-only argument:\n\n  ..., *, cache\u003dTrue):","commit_id":"43683f543ec196502ed13192518e4eb89be8868c"}]}