)]}'
{"nova/tests/unit/virt/libvirt/fake_imagebackend.py":[{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"1a28b7440792e669b1552bbe70ba9ce9364ecf21","unresolved":false,"context_lines":[{"line_number":34,"context_line":"            def lock(self):"},{"line_number":35,"context_line":"                self.locked \u003d True"},{"line_number":36,"context_line":"                yield"},{"line_number":37,"context_line":"                self.locked \u003d False"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"            def create_image(self, prepare_template, base,"},{"line_number":40,"context_line":"                              size, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":6,"id":"1a122d0e_e1384ca2","line":37,"updated":"2016-04-29 14:15:06.000000000","message":"This seems to be a peculiar way to ensure mutual exclusivity to me. If a thread calls the lock() method and sets locked to \u0027True\u0027, there doesn\u0027t seem to be anything preventing another thread with another context from doing the same thing. Why not use the threading library\u0027s standard implementation of a semaphore?","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"80a04714b06f9e5f8e815b8b92059d1c618c93da","unresolved":false,"context_lines":[{"line_number":34,"context_line":"            def lock(self):"},{"line_number":35,"context_line":"                self.locked \u003d True"},{"line_number":36,"context_line":"                yield"},{"line_number":37,"context_line":"                self.locked \u003d False"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"            def create_image(self, prepare_template, base,"},{"line_number":40,"context_line":"                              size, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_d109e096","line":37,"in_reply_to":"1a122d0e_e1384ca2","updated":"2016-05-16 14:43:25.000000000","message":"The purpose of this method isn\u0027t mutual exclusion; it\u0027s creating something which we can assert in a test. Note that in the actual (non-fake) implementation of this class, we update the re-entrance guard with the oslo lock held.","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"}],"nova/tests/unit/virt/libvirt/test_imagebackend.py":[{"author":{"_account_id":15524,"name":"dane-fichter","email":"dane.fichter@jhuapl.edu","username":"dane-fichter"},"change_message_id":"1a28b7440792e669b1552bbe70ba9ce9364ecf21","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            try:"},{"line_number":71,"context_line":"                yield"},{"line_number":72,"context_line":"            finally:"},{"line_number":73,"context_line":"                self.locked -\u003d 1"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"        lock_patcher \u003d mock.patch.object(lockutils, \u0027lock\u0027,"},{"line_number":76,"context_line":"                                         autospec\u003dTrue, side_effect\u003dfake_lock)"}],"source_content_type":"text/x-python","patch_set":6,"id":"1a122d0e_bdd95624","line":73,"updated":"2016-04-29 14:15:06.000000000","message":"Why does the fake lock use 0/1 instead of True/False like the other locks? Not saying it\u0027s an issue just curious.","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"}],"nova/virt/libvirt/imagebackend.py":[{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"53c247c059d6c0c70e34f20f0e3855cad1b7c56a","unresolved":false,"context_lines":[{"line_number":435,"context_line":""},{"line_number":436,"context_line":"            # This MUST be compatible with the lock used by"},{"line_number":437,"context_line":"            # nova.utils.synchronized()."},{"line_number":438,"context_line":"            # NOTE(mdbooth): Ideally there would be a way create a context"},{"line_number":439,"context_line":"            # manager which uses nova.utils.synchronized directly."},{"line_number":440,"context_line":"            # Unfortunately I can\u0027t find one."},{"line_number":441,"context_line":"            with lockutils.lock(lock_name, lock_file_prefix\u003d\u0027nova-\u0027,"},{"line_number":442,"context_line":"                                external\u003dTrue,"},{"line_number":443,"context_line":"                                lock_path\u003dself.lock_path):"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_11ba830a","line":440,"range":{"start_line":438,"start_character":0,"end_line":440,"end_character":45},"updated":"2016-05-13 21:44:04.000000000","message":"Could you please explain why you cannot (or do not want to) use nova.utils.synchronized directly with function closures as it does currently in this module?\n\nI looked at the end of this patch series and found that two ways are used simultaneously in this module as the result. I think it will be hard to explain to developers which way is preferred to use with further changes. At least i do not understand it. Except the fact that a closure requires more lines. But this does not explain me why the decorator continues to be used here in this module.","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"80a04714b06f9e5f8e815b8b92059d1c618c93da","unresolved":false,"context_lines":[{"line_number":435,"context_line":""},{"line_number":436,"context_line":"            # This MUST be compatible with the lock used by"},{"line_number":437,"context_line":"            # nova.utils.synchronized()."},{"line_number":438,"context_line":"            # NOTE(mdbooth): Ideally there would be a way create a context"},{"line_number":439,"context_line":"            # manager which uses nova.utils.synchronized directly."},{"line_number":440,"context_line":"            # Unfortunately I can\u0027t find one."},{"line_number":441,"context_line":"            with lockutils.lock(lock_name, lock_file_prefix\u003d\u0027nova-\u0027,"},{"line_number":442,"context_line":"                                external\u003dTrue,"},{"line_number":443,"context_line":"                                lock_path\u003dself.lock_path):"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_c66804f8","line":440,"range":{"start_line":438,"start_character":0,"end_line":440,"end_character":45},"in_reply_to":"dab17558_11ba830a","updated":"2016-05-16 14:43:25.000000000","message":"A context manager is the natural fit for locking in this case, where we want to explicitly lock a particular object for a certain scope. In fact, nothing else really makes sense. The use of the decorator in this module is generally problematic and hard to follow[1], and will be removed gradually.\n\n[1] Audit Image.cache() and prove to me that we always lock the image cache file while updating the image cache, and the ephemeral disk file while updating the ephemeral disk. It\u0027s not obvious! (Although I think it\u0027s correct, btw.)","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"e841b5d68ab9d16b322b26e9b9aa20f11796ffa0","unresolved":false,"context_lines":[{"line_number":435,"context_line":""},{"line_number":436,"context_line":"            # This MUST be compatible with the lock used by"},{"line_number":437,"context_line":"            # nova.utils.synchronized()."},{"line_number":438,"context_line":"            # NOTE(mdbooth): Ideally there would be a way create a context"},{"line_number":439,"context_line":"            # manager which uses nova.utils.synchronized directly."},{"line_number":440,"context_line":"            # Unfortunately I can\u0027t find one."},{"line_number":441,"context_line":"            with lockutils.lock(lock_name, lock_file_prefix\u003d\u0027nova-\u0027,"},{"line_number":442,"context_line":"                                external\u003dTrue,"},{"line_number":443,"context_line":"                                lock_path\u003dself.lock_path):"}],"source_content_type":"text/x-python","patch_set":6,"id":"bab6814e_dde2089f","line":440,"range":{"start_line":438,"start_character":0,"end_line":440,"end_character":45},"in_reply_to":"dab17558_c66804f8","updated":"2016-05-18 16:37:38.000000000","message":"Well, perhaps it makes sense to switch the code to a new locking engine. But what \u0027gradually\u0027 does mean? Is the code going to be cleaned from the decorador in this patch series, or you assume volunteers to do that sometimes later?\n\nI believe if a refactoring introduces something new, this new must be set instead of an old thing throughout the code. Otherwise it is not a refactoring, but an increase of entropy.\n\nIf you was going to switch the code to this lock, it would be perfect to get a patch subseries, where this review is the first, the next one (or several ones) switches current code to the new lock, and then you use the lock for the main refactoring goals.","commit_id":"c32b08a5c6e18867badb0800ab109353f243ba1a"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"01aadbb18c7295913624610c9c089f1f9b36d92a","unresolved":false,"context_lines":[{"line_number":403,"context_line":""},{"line_number":404,"context_line":"            # This MUST be compatible with the lock used by"},{"line_number":405,"context_line":"            # nova.utils.synchronized()."},{"line_number":406,"context_line":"            # NOTE(mdbooth): Ideally there would be a way create a context"},{"line_number":407,"context_line":"            # manager which uses nova.utils.synchronized directly."},{"line_number":408,"context_line":"            # Unfortunately I can\u0027t find one."},{"line_number":409,"context_line":"            with lockutils.lock(lock_name, lock_file_prefix\u003d\u0027nova-\u0027,"},{"line_number":410,"context_line":"                                external\u003dTrue,"},{"line_number":411,"context_line":"                                lock_path\u003dself.lock_path):"}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_51a40bb8","line":408,"range":{"start_line":406,"start_character":0,"end_line":408,"end_character":45},"updated":"2016-05-13 21:51:44.000000000","message":"Could you please explain why you cannot (or do not want to) use nova.utils.synchronized directly with function closures as it does currently in this module?\n\nI looked at the end of this patch series and found that two ways are used simultaneously in this module as the result. I think it will be hard to explain to developers which way is preferred to use with further changes. At least i do not understand it. Except the fact that a closure requires more lines. But this does not explain me why the decorator continues to be used here in this module.","commit_id":"126926c0688940817934acc4f9bcdb18a2bccd50"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"98318858e1dcfbc1e783eae6a841f840a09eed7c","unresolved":false,"context_lines":[{"line_number":394,"context_line":"        # We protect here against re-entrance. We assume that this object"},{"line_number":395,"context_line":"        # isn\u0027t shared between multiple threads. The actual lock is"},{"line_number":396,"context_line":"        # independent of this object, so multiple threads/processes is safe"},{"line_number":397,"context_line":"        # as long as each has their own object."},{"line_number":398,"context_line":"        if self.locked:"},{"line_number":399,"context_line":"            yield"},{"line_number":400,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_c98d327d","line":397,"updated":"2016-05-18 15:27:36.000000000","message":"Do we explicitly want to support nesting this lock? If not, I would rather we raise here to make it clear where we\u0027ve grabbed the lock again unexpectedly.\n\nAlso, something else that holds the lock via synchronized (per below) might assume that since this lock is reentrant that they can call this method, which they can\u0027t.","commit_id":"b9e9f3e647f5eb3dab718fdb44535f0547cc67ca"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"98318858e1dcfbc1e783eae6a841f840a09eed7c","unresolved":false,"context_lines":[{"line_number":408,"context_line":"            # Unfortunately I can\u0027t find one."},{"line_number":409,"context_line":"            with lockutils.lock(lock_name, lock_file_prefix\u003d\u0027nova-\u0027,"},{"line_number":410,"context_line":"                                external\u003dTrue,"},{"line_number":411,"context_line":"                                lock_path\u003dself.lock_path):"},{"line_number":412,"context_line":"                self.locked \u003d True"},{"line_number":413,"context_line":"                try:"},{"line_number":414,"context_line":"                    yield"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_49f822e6","line":411,"updated":"2016-05-18 15:27:36.000000000","message":"By my reading of the code in lockutils, you don\u0027t end up with an external lock if you use nova.utils.synchronized. Thus, this won\u0027t ever be compatible with it and actually provide exclusion.\n\nWherever else it is that uses synchronized() probably needs to change to use this instead of trying to make this compatible. Or, define another thing that this and some new decorator uses that you can replace on the affected bits.","commit_id":"b9e9f3e647f5eb3dab718fdb44535f0547cc67ca"}]}