)]}'
{"nova/api/metadata/handler.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"870559b8bb840660aef4883a96bbc891bca0d806","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            raise exception.FixedIpNotFoundForAddress(address\u003daddress)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"        cache_key \u003d \u0027metadata-%s\u0027 % address"},{"line_number":86,"context_line":"        with self._cache_lock[cache_key]:"},{"line_number":87,"context_line":"            data \u003d self._cache.get(cache_key)"},{"line_number":88,"context_line":"            if data:"},{"line_number":89,"context_line":"                LOG.debug(\"Using cached metadata for %s\", address)"}],"source_content_type":"text/x-python","patch_set":2,"id":"7af24918_d8e8a0b5","line":86,"updated":"2016-03-04 15:58:19.000000000","message":"Please use the @util.sychronised(cache_key) pattern we use elsewhere in nova:\nhttps://github.com/openstack/nova/blob/1635086529c43edcb69966c48c41b5fca2d1b26c/nova/utils.py#L173\n\nHonestly, its also tempting to only use the lock when the cache is empty, and double check the cache is empty after acquiring the lock.","commit_id":"ed2d558e8d560a28be28a9cc5a541ebdbcd30062"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"762f11c90e326b6e22bafc1059dcd9168d9163e3","unresolved":false,"context_lines":[{"line_number":87,"context_line":"        if data:"},{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_b9e4860d","line":90,"updated":"2016-03-07 21:10:23.000000000","message":"Without external\u003dTrue, this will only work against other greenthreads in the same process. Since we run API workers as processes, this won\u0027t really help much.\n\nAlso note that this will definitely not help for services running on multiple nodes, which I think is how you do it by default in neutron. So we\u0027ll be spreading requests among the nodes running api, and they will only lock internal to that one node.","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1bd09c0eeb62eff17ec068414858b3b7f636ef38","unresolved":false,"context_lines":[{"line_number":87,"context_line":"        if data:"},{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_91e85112","line":90,"in_reply_to":"5aef4532_0f61d139","updated":"2016-03-08 17:53:30.000000000","message":"Why would we want one request per process (which could be 64 on a modest machine) when we could have one per machine by just adding external\u003dTrue ?","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":19173,"name":"Sven Anderson","email":"sven@redhat.com","username":"ansiwen"},"change_message_id":"b044dffac699254a52125f14595ec48ba2a382f6","unresolved":false,"context_lines":[{"line_number":87,"context_line":"        if data:"},{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_e5270ab4","line":90,"in_reply_to":"5aef4532_91e85112","updated":"2016-03-09 17:17:05.000000000","message":"Right. If no shared cache is used, it would still be 64, just serialized. But I guess that doesn\u0027t matter too much. Done.","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":19173,"name":"Sven Anderson","email":"sven@redhat.com","username":"ansiwen"},"change_message_id":"95b7c1add98ac38117b99fea1148faeeb66a4634","unresolved":false,"context_lines":[{"line_number":87,"context_line":"        if data:"},{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_0f61d139","line":90,"in_reply_to":"5aef4532_b9e4860d","updated":"2016-03-08 09:34:10.000000000","message":"Why wouldn\u0027t this help much? In case of request bursts (at which this kind of locking is aimed) this would reduce the load to 1 request per process instead of 100s of requests per process. Also see the related change, that disables memcached for metadata, so each process would need to populate it\u0027s own cache anyway.","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":12712,"name":"Gleb Stepanov","email":"gstepanov@mirantis.com","username":"stgleb"},"change_message_id":"e12e08686b3d78c16476d1d6a97959f888a40aa3","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"},{"line_number":94,"context_line":"            if data:"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_1e2beeec","line":91,"updated":"2016-03-07 16:32:58.000000000","message":"Maybe you should implement some tests for new methods?","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":19173,"name":"Sven Anderson","email":"sven@redhat.com","username":"ansiwen"},"change_message_id":"95b7c1add98ac38117b99fea1148faeeb66a4634","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            return data"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"        @utils.synchronized(cache_key)"},{"line_number":91,"context_line":"        def _get_synchronized():"},{"line_number":92,"context_line":"            # check cache again in case it has been populated in the meantime"},{"line_number":93,"context_line":"            data \u003d self._get_metadata_from_cache(cache_key)"},{"line_number":94,"context_line":"            if data:"}],"source_content_type":"text/x-python","patch_set":4,"id":"5aef4532_873a0e35","line":91,"in_reply_to":"5aef4532_1e2beeec","updated":"2016-03-08 09:34:10.000000000","message":"I don\u0027t think so. It\u0027s a private method and therefore an implementation detail and has full coverage by the tests of get_metadata_by*() methods. It\u0027s not the more the better.","commit_id":"e65d0e4e5c6eb5b733337812e09885699eca0a74"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1200767d32bf53362f59c0aaeba6ae243e422fa5","unresolved":false,"context_lines":[{"line_number":88,"context_line":"        if data:"},{"line_number":89,"context_line":"            return data"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        lock_path \u003d os.path.join(CONF.instances_path, \u0027locks\u0027)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        @utils.synchronized(cache_key, external\u003dTrue, lock_path\u003dlock_path)"},{"line_number":94,"context_line":"        def _get_synchronized():"}],"source_content_type":"text/x-python","patch_set":6,"id":"5aef4532_d587db5e","line":91,"updated":"2016-03-10 17:55:18.000000000","message":"I don\u0027t think this is always present on API nodes currently, if you run this on dedicated API nodes (which I think you do for neutron?).\n\nCan\u0027t we use the default lock path for the concurrency utils?\n\nIf we start using this new config, and the obvious requirements on permissions, we should add a reno note to this change, as an upgrade note.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8efa5b8e54d571748cf89588842bc931fa09dc4d","unresolved":false,"context_lines":[{"line_number":88,"context_line":"        if data:"},{"line_number":89,"context_line":"            return data"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        lock_path \u003d os.path.join(CONF.instances_path, \u0027locks\u0027)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        @utils.synchronized(cache_key, external\u003dTrue, lock_path\u003dlock_path)"},{"line_number":94,"context_line":"        def _get_synchronized():"}],"source_content_type":"text/x-python","patch_set":6,"id":"3afc51ec_da5b1bf5","line":91,"in_reply_to":"5aef4532_4052de67","updated":"2016-03-14 12:23:06.000000000","message":"The bigger problem here is using a local lock with a distributed cache. Locking should be pushed into the caching client, which can at least choose to get it wrong explicitly.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4a59218420b4f305f56ca0627444b5ece857d481","unresolved":false,"context_lines":[{"line_number":88,"context_line":"        if data:"},{"line_number":89,"context_line":"            return data"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        lock_path \u003d os.path.join(CONF.instances_path, \u0027locks\u0027)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        @utils.synchronized(cache_key, external\u003dTrue, lock_path\u003dlock_path)"},{"line_number":94,"context_line":"        def _get_synchronized():"}],"source_content_type":"text/x-python","patch_set":6,"id":"5aef4532_4052de67","line":91,"in_reply_to":"5aef4532_d587db5e","updated":"2016-03-10 21:15:53.000000000","message":"This is definitely not present on typical API nodes. Agree that we should be using whatever concurrency utils defaults to.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1200767d32bf53362f59c0aaeba6ae243e422fa5","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            except exception.NotFound:"},{"line_number":103,"context_line":"                return None"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"            if CONF.metadata_cache_expiration \u003e 0:"},{"line_number":106,"context_line":"                self._cache.set(cache_key, data)"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"            return data"}],"source_content_type":"text/x-python","patch_set":6,"id":"5aef4532_d5f35b71","line":105,"updated":"2016-03-10 17:55:18.000000000","message":"Hmm, maybe this is too much for this change, but should this path not really be outside the locking, and should not really call the cache first? Just check this, and call expensive_getter directly?","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8efa5b8e54d571748cf89588842bc931fa09dc4d","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            except exception.NotFound:"},{"line_number":103,"context_line":"                return None"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"            if CONF.metadata_cache_expiration \u003e 0:"},{"line_number":106,"context_line":"                self._cache.set(cache_key, data)"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"            return data"}],"source_content_type":"text/x-python","patch_set":6,"id":"3afc51ec_6f8fabcc","line":105,"in_reply_to":"3afc51ec_46ce635b","updated":"2016-03-14 12:23:06.000000000","message":"This is an optimisation, rather than a use of the cache expiration value. If metadata_cache_expiration \u003d\u003d 0, then the cache is disabled. This code just shortcuts the cache in this case. It\u0027s not incorrect.\n\nHowever, I don\u0027t like it because now we\u0027ve encoded \u0027metadata_cache_expiration \u003d\u003d 0\u0027 \u003d\u003d \u0027cache disabled\u0027 in more than 1 place. I would like to see this removed and have the value passed to _cache unconditionally. The cache client is then free to handle a zero expiration efficiently. Sure it\u0027s an extra function call, but until that shows up in perf I\u0027ll take more maintainable code.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":12712,"name":"Gleb Stepanov","email":"gstepanov@mirantis.com","username":"stgleb"},"change_message_id":"1eeafcb2c3e75fb8ce5340dd698aeb5e75594cd8","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            except exception.NotFound:"},{"line_number":103,"context_line":"                return None"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"            if CONF.metadata_cache_expiration \u003e 0:"},{"line_number":106,"context_line":"                self._cache.set(cache_key, data)"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"            return data"}],"source_content_type":"text/x-python","patch_set":6,"id":"5aef4532_dfdb5620","line":105,"in_reply_to":"5aef4532_d5f35b71","updated":"2016-03-11 10:49:14.000000000","message":"But if you do _cache,set(key, value) outside of synchronized block, another thread can interrupt you and populate cache value.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"7e58fcade104a9ff2487085fe2500e12cb2c1697","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            except exception.NotFound:"},{"line_number":103,"context_line":"                return None"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"            if CONF.metadata_cache_expiration \u003e 0:"},{"line_number":106,"context_line":"                self._cache.set(cache_key, data)"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"            return data"}],"source_content_type":"text/x-python","patch_set":6,"id":"3afc51ec_46ce635b","line":105,"in_reply_to":"5aef4532_dfdb5620","updated":"2016-03-11 13:38:36.000000000","message":"I don\u0027t mean that.\n\nI mean, when caching is disabled, by sure to call expensive_getter() outside this block.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8efa5b8e54d571748cf89588842bc931fa09dc4d","unresolved":false,"context_lines":[{"line_number":115,"context_line":""},{"line_number":116,"context_line":"        cache_key \u003d \u0027metadata-%s\u0027 % address"},{"line_number":117,"context_line":"        expensive_getter \u003d partial(base.get_metadata_by_address,"},{"line_number":118,"context_line":"                                             address)"},{"line_number":119,"context_line":"        return self._get_metadata(cache_key, expensive_getter)"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def get_metadata_by_instance_id(self, instance_id, address):"}],"source_content_type":"text/x-python","patch_set":6,"id":"3afc51ec_cf4d1f74","line":118,"updated":"2016-03-14 12:23:06.000000000","message":"nit: style thing, really. For something simple like this where you\u0027re creating a function which takes no arguments, I\u0027d use a lambda instead of partial(), which is more general and therefore a bit harder to read. Passing a lambda makes it very obvious that the resulting function takes no arguments, and also easier to see what it does.\n\nI wouldn\u0027t -1 for this if you didn\u0027t want to change it.","commit_id":"3dd0577c3fc7d25b403b01d66d259c35b1677528"}]}