)]}'
{"nova/db/sqlalchemy/cache.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":177,"context_line":"        self.ns_cache \u003d {}"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":"    def _refresh_cache(self):"},{"line_number":180,"context_line":"        _refresh_from_db(self.ctx, _TRAIT_TBL, self)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        for trait in self.str_cache.values():"},{"line_number":183,"context_line":"            namespaces \u003d trait.split(\u0027_\u0027)[:-1]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_2e6fc5fc","line":180,"updated":"2017-03-06 11:37:21.000000000","message":"Now I see why _refresh_from_db from the previous patch remains outside the subclass.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":177,"context_line":"        self.ns_cache \u003d {}"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":"    def _refresh_cache(self):"},{"line_number":180,"context_line":"        _refresh_from_db(self.ctx, _TRAIT_TBL, self)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        for trait in self.str_cache.values():"},{"line_number":183,"context_line":"            namespaces \u003d trait.split(\u0027_\u0027)[:-1]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_c3919a48","line":180,"in_reply_to":"ba2be162_2e6fc5fc","updated":"2017-03-06 13:18:56.000000000","message":"yea, I should have some explain in the commit message.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        _refresh_from_db(self.ctx, _TRAIT_TBL, self)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        for trait in self.str_cache.values():"},{"line_number":183,"context_line":"            namespaces \u003d trait.split(\u0027_\u0027)[:-1]"},{"line_number":184,"context_line":"            previous_ns \u003d \u0027\u0027"},{"line_number":185,"context_line":"            for ns in namespaces:"},{"line_number":186,"context_line":"                if previous_ns:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_4ec79995","line":183,"updated":"2017-03-06 11:37:21.000000000","message":"With parsing code like this, I\u0027d prefer a comment up top briefly explaining expected inputs and outputs. Makes it simpler for both the reviewer and maintainer.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        _refresh_from_db(self.ctx, _TRAIT_TBL, self)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        for trait in self.str_cache.values():"},{"line_number":183,"context_line":"            namespaces \u003d trait.split(\u0027_\u0027)[:-1]"},{"line_number":184,"context_line":"            previous_ns \u003d \u0027\u0027"},{"line_number":185,"context_line":"            for ns in namespaces:"},{"line_number":186,"context_line":"                if previous_ns:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_03981269","line":183,"in_reply_to":"ba2be162_4ec79995","updated":"2017-03-06 13:18:56.000000000","message":"yea, I will add a comment.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":185,"context_line":"            for ns in namespaces:"},{"line_number":186,"context_line":"                if previous_ns:"},{"line_number":187,"context_line":"                    ns \u003d \u0027%s_%s\u0027 % (previous_ns, ns)"},{"line_number":188,"context_line":"                if ns in self.ns_cache:"},{"line_number":189,"context_line":"                    self.ns_cache[ns].append(trait)"},{"line_number":190,"context_line":"                else:"},{"line_number":191,"context_line":"                    self.ns_cache[ns] \u003d [trait]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_4e05d943","line":188,"updated":"2017-03-06 11:37:21.000000000","message":"Suggestion: if ns_cache was a defaultdict(list) you could just unconditionally append to it here, which would make this code slightly easier to read.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":185,"context_line":"            for ns in namespaces:"},{"line_number":186,"context_line":"                if previous_ns:"},{"line_number":187,"context_line":"                    ns \u003d \u0027%s_%s\u0027 % (previous_ns, ns)"},{"line_number":188,"context_line":"                if ns in self.ns_cache:"},{"line_number":189,"context_line":"                    self.ns_cache[ns].append(trait)"},{"line_number":190,"context_line":"                else:"},{"line_number":191,"context_line":"                    self.ns_cache[ns] \u003d [trait]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_239b5667","line":188,"in_reply_to":"ba2be162_4e05d943","updated":"2017-03-06 13:18:56.000000000","message":"ok, will do","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":189,"context_line":"                    self.ns_cache[ns].append(trait)"},{"line_number":190,"context_line":"                else:"},{"line_number":191,"context_line":"                    self.ns_cache[ns] \u003d [trait]"},{"line_number":192,"context_line":"                previous_ns \u003d ns"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def strings_from_prefix(self, prefix):"},{"line_number":195,"context_line":"        with lockutils.lock(self.LOCK_NAME):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_0ed041c8","line":192,"updated":"2017-03-06 11:37:21.000000000","message":"So, looks like if we\u0027ve got a trait called \u0027foo_bar_baz\u0027 we\u0027ll end up with ns_cache:\n\n{\u0027foo\u0027: [\u0027foo_bar_baz\u0027], \u0027foo_bar\u0027: [\u0027foo_bar_baz\u0027]}","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":189,"context_line":"                    self.ns_cache[ns].append(trait)"},{"line_number":190,"context_line":"                else:"},{"line_number":191,"context_line":"                    self.ns_cache[ns] \u003d [trait]"},{"line_number":192,"context_line":"                previous_ns \u003d ns"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def strings_from_prefix(self, prefix):"},{"line_number":195,"context_line":"        with lockutils.lock(self.LOCK_NAME):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_43a68aae","line":192,"in_reply_to":"ba2be162_0ed041c8","updated":"2017-03-06 13:18:56.000000000","message":"yes, it is similar to tree structure.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        with lockutils.lock(self.LOCK_NAME):"},{"line_number":196,"context_line":"            # Refresh the cache when the namespace cache is empty."},{"line_number":197,"context_line":"            if not self.ns_cache:"},{"line_number":198,"context_line":"                self._refresh_cache()"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"            # empty prefix means return everything"},{"line_number":201,"context_line":"            if prefix \u003d\u003d \u0027\u0027:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_9119b024","line":198,"updated":"2017-03-06 11:37:21.000000000","message":"Unlike with id_from_string and string_from_id, a miss here will never force a refresh to fetch custom traits added since the service started. This doesn\u0027t look correct.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        with lockutils.lock(self.LOCK_NAME):"},{"line_number":196,"context_line":"            # Refresh the cache when the namespace cache is empty."},{"line_number":197,"context_line":"            if not self.ns_cache:"},{"line_number":198,"context_line":"                self._refresh_cache()"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"            # empty prefix means return everything"},{"line_number":201,"context_line":"            if prefix \u003d\u003d \u0027\u0027:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_63cf0e56","line":198,"in_reply_to":"ba2be162_9119b024","updated":"2017-03-06 13:18:56.000000000","message":"This method is just filtering by the prefix, there is no way to say \u0027miss\u0027.  So I have a cache cleanup in the create/delete trait at https://review.openstack.org/#/c/376199/17/nova/objects/resource_provider.py@1480","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":210,"context_line":"                        return self.ns_cache[ns]"},{"line_number":211,"context_line":"                    else:"},{"line_number":212,"context_line":"                        return [trait for trait in self.ns_cache[ns]"},{"line_number":213,"context_line":"                                    if trait.startswith(prefix)]"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"            return [prefix] if prefix in self.id_cache else []"},{"line_number":216,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_b41a22c4","line":213,"updated":"2017-03-06 11:37:21.000000000","message":"If we\u0027ve gone to the trouble above of indexing by all prefix combinations, why wouldn\u0027t we lookup the full prefix?","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"6f30001106dd0d69d46ad88c4871852d98dbe0b4","unresolved":false,"context_lines":[{"line_number":210,"context_line":"                        return self.ns_cache[ns]"},{"line_number":211,"context_line":"                    else:"},{"line_number":212,"context_line":"                        return [trait for trait in self.ns_cache[ns]"},{"line_number":213,"context_line":"                                    if trait.startswith(prefix)]"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"            return [prefix] if prefix in self.id_cache else []"},{"line_number":216,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9a30ddce_168922cb","line":213,"in_reply_to":"ba2be162_249999c4","updated":"2017-03-07 02:08:15.000000000","message":"Ah, I totally missed those thing, it\u0027s only going to work with namespace match, thanks. Yea, looks like have to introduce a tree. But yes, I don\u0027t think we will have very big list. So maybe let us start with ordered list is enough.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c4957c2e5d967596742b681fb58dbe940635f8ea","unresolved":false,"context_lines":[{"line_number":210,"context_line":"                        return self.ns_cache[ns]"},{"line_number":211,"context_line":"                    else:"},{"line_number":212,"context_line":"                        return [trait for trait in self.ns_cache[ns]"},{"line_number":213,"context_line":"                                    if trait.startswith(prefix)]"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"            return [prefix] if prefix in self.id_cache else []"},{"line_number":216,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_249999c4","line":213,"in_reply_to":"ba2be162_43de8add","updated":"2017-03-06 15:11:29.000000000","message":"Ah, I\u0027ve just had a look at the following patch. Couple of things:\n\nFirstly, the \u0027namespace\u0027 idea really only makes sense if we\u0027re doing whole word matching, not arbitrary matching. If we\u0027re doing namespaces, and stopping at the first prefix match like this, then ns_cache has the wrong structure. Say we\u0027ve got a trait called \u0027FOO_BAR_BAZ\u0027. We\u0027ve split that into \u0027FOO\u0027, and \u0027FOO_BAR\u0027. However, because we\u0027re doing a shortest prefix match in this loop, we will only ever match \u0027FOO\u0027, and then do another prefix match on everything in \u0027FOO\u0027. \u0027FOO_BAR\u0027 will never be touched, so this isn\u0027t right.\n\nSecondly, it looks like we\u0027re really just doing prefix matches not namespaces. This means that with this code if we\u0027ve got a traits called \u0027FOO_BAR_BAZ\u0027 and \u0027FOL_BAR_BAZ\u0027, and the user does a query startswith:\u0027F\u0027, they\u0027re only going to be returned \u0027FOO_BAR_BAZ\u0027 because we\u0027ve already incorrectly split our data based on the \u0027_\u0027 character, which isn\u0027t part of our semantics.\n\nI think you need a completely different approach to ns_cache. If the list is going to be big you want something like a tree (the character \u0027_\u0027 will not be involved). If it\u0027s only going to be a few 10s of entries at most, you probably just want to keep an ordered list and scan it for prefix matches.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":210,"context_line":"                        return self.ns_cache[ns]"},{"line_number":211,"context_line":"                    else:"},{"line_number":212,"context_line":"                        return [trait for trait in self.ns_cache[ns]"},{"line_number":213,"context_line":"                                    if trait.startswith(prefix)]"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"            return [prefix] if prefix in self.id_cache else []"},{"line_number":216,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_43de8add","line":213,"in_reply_to":"ba2be162_b41a22c4","updated":"2017-03-06 13:18:56.000000000","message":"in case we have CUSTOM_ABC, CUSTOM_ADE, CUSTOM_AXY. Then prefix is CUSTOM_A.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9a24e794646e7fccb144f43a9618dd0935e37c7","unresolved":false,"context_lines":[{"line_number":216,"context_line":""},{"line_number":217,"context_line":"    def clear(self):"},{"line_number":218,"context_line":"        super(TraitCache, self).clear()"},{"line_number":219,"context_line":"        self.ns_cache \u003d {}"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_ee368d01","line":219,"updated":"2017-03-06 11:37:21.000000000","message":"Modification of ns_cache needs to be locked.","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"18390c2283dcc401e39e8326d69c2089cc4b8857","unresolved":false,"context_lines":[{"line_number":216,"context_line":""},{"line_number":217,"context_line":"    def clear(self):"},{"line_number":218,"context_line":"        super(TraitCache, self).clear()"},{"line_number":219,"context_line":"        self.ns_cache \u003d {}"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba2be162_63d9cef2","line":219,"in_reply_to":"ba2be162_ee368d01","updated":"2017-03-06 13:18:56.000000000","message":"thanks for this nice cache!","commit_id":"47ec57b4140a3b10f13834e18ef7c151121900f4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e57c17f1a23be105310afc40d67a02cfb8c4deb8","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        with lockutils.lock(self.LOCK_NAME):"},{"line_number":182,"context_line":"            # If the cache is empty, it means that the cache need to update."},{"line_number":183,"context_line":"            if not self.str_cache:"},{"line_number":184,"context_line":"                self._refresh_cache()"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"            for string in self.str_cache.values():"},{"line_number":187,"context_line":"                if string.startswith(prefix):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a30ddce_330e2635","line":184,"updated":"2017-03-07 10:04:24.000000000","message":"This still doesn\u0027t look right to me. Looking at the next patch, strings_from_prefix is called by get_all(). Consider a warm system where str_cache is already populated. A user does:\n\n1. Add a new trait \u0027FOO\u0027\n2. get_all(prefix\u003d\u0027F\u0027)\n\nget_all won\u0027t return the newly added \u0027FOO\u0027 unless some other operation happens to call id_from_string() or string_from_id() first, because the cache won\u0027t be refreshed.","commit_id":"b5065dd8db266be97e0f9916914833d359d2ae36"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"8d41c5da2e9b84406fad5ec58afc09cd5a3dfebf","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        with lockutils.lock(self.LOCK_NAME):"},{"line_number":182,"context_line":"            # If the cache is empty, it means that the cache need to update."},{"line_number":183,"context_line":"            if not self.str_cache:"},{"line_number":184,"context_line":"                self._refresh_cache()"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"            for string in self.str_cache.values():"},{"line_number":187,"context_line":"                if string.startswith(prefix):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a30ddce_e24bd9ae","line":184,"in_reply_to":"9a30ddce_330e2635","updated":"2017-03-07 11:03:02.000000000","message":"I have a cache cleanup when add new trait https://review.openstack.org/#/c/376199/19/nova/objects/resource_provider.py@1533\n\nSo it will trigger this cache refresh.","commit_id":"b5065dd8db266be97e0f9916914833d359d2ae36"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a0e2a57af410a2ba93ba09bb1deced21f9448843","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        with lockutils.lock(self.LOCK_NAME):"},{"line_number":182,"context_line":"            # If the cache is empty, it means that the cache need to update."},{"line_number":183,"context_line":"            if not self.str_cache:"},{"line_number":184,"context_line":"                self._refresh_cache()"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"            for string in self.str_cache.values():"},{"line_number":187,"context_line":"                if string.startswith(prefix):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a30ddce_f84c6678","line":184,"in_reply_to":"9a30ddce_e24bd9ae","updated":"2017-03-07 11:32:26.000000000","message":"I see. However, as discussed on IRC with cdent, this won\u0027t work if there are multiple api servers. If the create is sent to server A, and the get_all() to server B, B will still have an out-of-date cache.","commit_id":"b5065dd8db266be97e0f9916914833d359d2ae36"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e57c17f1a23be105310afc40d67a02cfb8c4deb8","unresolved":false,"context_lines":[{"line_number":185,"context_line":""},{"line_number":186,"context_line":"            for string in self.str_cache.values():"},{"line_number":187,"context_line":"                if string.startswith(prefix):"},{"line_number":188,"context_line":"                    ret.append(string)"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        return ret"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a30ddce_08e8117c","line":188,"updated":"2017-03-07 10:04:24.000000000","message":"Wow, that shrinks the code a bit :)\n\nJust to check: we\u0027re really not expecting more than hundreds of traits at most, even on a very large system? I don\u0027t have the context to answer this. Assuming not, though, I don\u0027t see any reason to get more complicated than this.\n\nSuggestion: you could replace 179, 186-190 with a list comprehension:\n\n  return [s for s in self.str_cache.values()\n          where s.startswith(prefix)]","commit_id":"b5065dd8db266be97e0f9916914833d359d2ae36"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"8d41c5da2e9b84406fad5ec58afc09cd5a3dfebf","unresolved":false,"context_lines":[{"line_number":185,"context_line":""},{"line_number":186,"context_line":"            for string in self.str_cache.values():"},{"line_number":187,"context_line":"                if string.startswith(prefix):"},{"line_number":188,"context_line":"                    ret.append(string)"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        return ret"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a30ddce_42b6658b","line":188,"in_reply_to":"9a30ddce_08e8117c","updated":"2017-03-07 11:03:02.000000000","message":"yea, :)\n\nAt least for now, we only have very few. Let us see if any people complain about this.\n\nAnd thanks for the suggestion!","commit_id":"b5065dd8db266be97e0f9916914833d359d2ae36"}]}