)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"6c8cfd451a4a3a894ee745a36e12a4e6adf69373","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Gregory Haynes \u003cgreg@greghaynes.net\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-09-11 22:52:49 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Make HashRings Singleton"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"We need to use hash rings outside of rpcapi. Furthermore, they do not"},{"line_number":10,"context_line":"need to be constructed per-request."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"fa98f980_6ab17000","line":7,"updated":"2014-09-12 09:35:25.000000000","message":"super nit: I think the terminlogy is not correct here. Singleton applies for classes it means that the class will be instantiated only once and every time someone construct the class it will return the same object.\n\nWhere in this case when you instantiate the HashRingManager class you will get different objects all the time, BUT, we are making sure that the HashRing is built only once.","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"}],"ironic/common/hash_ring.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"8c24685f372e63133bf50dba5a4418559e2df799","unresolved":false,"context_lines":[{"line_number":114,"context_line":""},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"class HashRingManager(object):"},{"line_number":117,"context_line":"    _class_state \u003d {}"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self):"},{"line_number":120,"context_line":"        self._lock \u003d threading.Lock()"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_b5a1f8f9","line":117,"updated":"2014-09-12 11:44:00.000000000","message":"Maybe it\u0027s only me, but I would prefer just _hash_rings here and then access it like\n\n if self._hash_rings is None:\n   self.__class__._hash_rings \u003d ...\n\ndiscussable.","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"6c8cfd451a4a3a894ee745a36e12a4e6adf69373","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        except KeyError:"},{"line_number":129,"context_line":"            pass"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"        with self._lock:"},{"line_number":132,"context_line":"            rings \u003d self._load_hash_rings()"},{"line_number":133,"context_line":"            self._class_state[\u0027hash_rings\u0027] \u003d rings"},{"line_number":134,"context_line":"            return rings"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_4a0bccd5","line":131,"updated":"2014-09-12 09:35:25.000000000","message":"It\u0027s better to test after the lock if the hash_ring wasn\u0027t already populated, in case there\u0027s two access at the same time 1 goes in and load the hash ring the second one is waiting in the lock, when the first one finishes the second one then goes in and will load the hash ring again.\n\nSo I think this should be:\n\n with self._lock:\n     if \u0027hash_rings\u0027 in HashRingManager._class_state:\n         rings \u003d self._load_hash_rings()\n         ...","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"8c24685f372e63133bf50dba5a4418559e2df799","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        except KeyError:"},{"line_number":129,"context_line":"            pass"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"        with self._lock:"},{"line_number":132,"context_line":"            rings \u003d self._load_hash_rings()"},{"line_number":133,"context_line":"            self._class_state[\u0027hash_rings\u0027] \u003d rings"},{"line_number":134,"context_line":"            return rings"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_353fc8b5","line":131,"in_reply_to":"fa98f980_4a0bccd5","updated":"2014-09-12 11:44:00.000000000","message":"s/in/not in/","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"8c24685f372e63133bf50dba5a4418559e2df799","unresolved":false,"context_lines":[{"line_number":150,"context_line":""},{"line_number":151,"context_line":"    def reset(self):"},{"line_number":152,"context_line":"        try:"},{"line_number":153,"context_line":"            del self._class_state[\u0027hash_rings\u0027]"},{"line_number":154,"context_line":"        except KeyError:"},{"line_number":155,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_d554bcff","line":153,"updated":"2014-09-12 11:44:00.000000000","message":"this should be inside lock, otherwise it can happen in the middle of hash_rings() call","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"99a177bbedf450e93df2fa2bc3e1845b3b1c0c7f","unresolved":false,"context_lines":[{"line_number":117,"context_line":"    _hash_rings \u003d None"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self):"},{"line_number":120,"context_line":"        self._lock \u003d threading.Lock()"},{"line_number":121,"context_line":"        self.dbapi \u003d dbapi.get_instance()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa98f980_55ef77e9","line":120,"updated":"2014-09-17 13:34:14.000000000","message":"what is this lock actually locking? if it lock access to _hash_rings, it should be a class field","commit_id":"feeec3a34d8ce7fcd938973b7dac98d760f0a58d"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"fe60b80a66d3058a5d4117222f95a9e4949f9306","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        if self._hash_rings is not None:"},{"line_number":127,"context_line":"            return self._hash_rings"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"        with self._lock:"},{"line_number":130,"context_line":"            if self._hash_rings is None:"},{"line_number":131,"context_line":"                rings \u003d self._load_hash_rings()"},{"line_number":132,"context_line":"                self.__class__._hash_rings \u003d rings"}],"source_content_type":"text/x-python","patch_set":6,"id":"da9df570_0fe654e8","line":129,"updated":"2014-09-25 17:32:09.000000000","message":"if, somehow, more than one HashRingManager is instantiated, it\u0027s going to create a\nrace here -- each instance will have a different lock, but they\u0027ll all be modifying\nthe same class variable","commit_id":"feeec3a34d8ce7fcd938973b7dac98d760f0a58d"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"fe60b80a66d3058a5d4117222f95a9e4949f9306","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            rings[driver_name] \u003d HashRing(hosts)"},{"line_number":141,"context_line":"        return rings"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    def get_hash_ring(self, driver_name):"},{"line_number":144,"context_line":"        try:"},{"line_number":145,"context_line":"            return self.hash_rings[driver_name]"},{"line_number":146,"context_line":"        except KeyError:"}],"source_content_type":"text/x-python","patch_set":6,"id":"da9df570_2feb18b1","line":143,"updated":"2014-09-25 17:32:09.000000000","message":"with the addition of the property, this should become a private method","commit_id":"feeec3a34d8ce7fcd938973b7dac98d760f0a58d"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"e4188c5c1065ebd952cec1bf3681519b8c33da5a","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self):"},{"line_number":120,"context_line":"        # NOTE(deva): this lock is part of the instance, not class so it\u0027s"},{"line_number":121,"context_line":"        #             not actually going to protect the _hash_rings singleton"},{"line_number":122,"context_line":"        self._lock \u003d threading.Lock()"},{"line_number":123,"context_line":"        self.dbapi \u003d dbapi.get_instance()"},{"line_number":124,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_9924dfc7","line":121,"updated":"2014-09-29 07:02:05.000000000","message":"then why not move it to class level?\n\neven more: why have class at all? global things should leave on module level IMO","commit_id":"7b29280ce46ec583d19b0131e9caa66341e954e7"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"0a91b1c76b01ce893fd65646c33de2fc2589e37d","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self):"},{"line_number":120,"context_line":"        # NOTE(deva): this lock is part of the instance, not class so it\u0027s"},{"line_number":121,"context_line":"        #             not actually going to protect the _hash_rings singleton"},{"line_number":122,"context_line":"        self._lock \u003d threading.Lock()"},{"line_number":123,"context_line":"        self.dbapi \u003d dbapi.get_instance()"},{"line_number":124,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_aacdc388","line":121,"in_reply_to":"baa201ad_9924dfc7","updated":"2014-09-29 09:29:58.000000000","message":"hmm, yeah I don\u0027t quite get the idea of the lock here too... If the hash_ring is accessed at the class level (same hashring for all the instances) why we have a lock at the instance level? it\u0027s not going to lock/prevent anything happen to the hash ring since each instance will have a diff lock.\n\nNow unless we make this class fully singleton by returning the same instance object every time it\u0027s instantiated and not only having the hash ring shared across multiple instances I don\u0027t see why we need a lock at the instance level.","commit_id":"7b29280ce46ec583d19b0131e9caa66341e954e7"},{"author":{"_account_id":10626,"name":"Russell Haering","email":"russellhaering@gmail.com","username":"russell_h"},"change_message_id":"c13b3bfe1c2770108053408045246d21cff7ab33","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            rings[driver_name] \u003d HashRing(hosts)"},{"line_number":141,"context_line":"        return rings"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    @classmethod"},{"line_number":144,"context_line":"    def reset(self):"},{"line_number":145,"context_line":"        with self._lock:"},{"line_number":146,"context_line":"            self._hash_rings \u003d None"}],"source_content_type":"text/x-python","patch_set":11,"id":"baa201ad_2fdb89ee","line":143,"updated":"2014-09-30 04:43:38.000000000","message":"Are you sure you want @classmethod here?","commit_id":"9de2d21a96f440807b9822173de3fd3b6119fbe8"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"dab8dc6aa356ffcde56e526a042dc4c2a3dbd430","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            rings[driver_name] \u003d HashRing(hosts)"},{"line_number":141,"context_line":"        return rings"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    @classmethod"},{"line_number":144,"context_line":"    def reset(self):"},{"line_number":145,"context_line":"        with self._lock:"},{"line_number":146,"context_line":"            self._hash_rings \u003d None"}],"source_content_type":"text/x-python","patch_set":11,"id":"baa201ad_52515061","line":143,"in_reply_to":"baa201ad_2fdb89ee","updated":"2014-09-30 06:29:04.000000000","message":"Yes. Both _lock and _hash_rings are class properties.\n\nOh, but the parameter should be named \"cls\" not \"self\".","commit_id":"9de2d21a96f440807b9822173de3fd3b6119fbe8"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"c0731f0cc72dc4a39e73ad434758723b87661408","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            rings[driver_name] \u003d HashRing(hosts)"},{"line_number":141,"context_line":"        return rings"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    @classmethod"},{"line_number":144,"context_line":"    def reset(self):"},{"line_number":145,"context_line":"        with self._lock:"},{"line_number":146,"context_line":"            self._hash_rings \u003d None"}],"source_content_type":"text/x-python","patch_set":11,"id":"baa201ad_663776e4","line":143,"in_reply_to":"baa201ad_52515061","updated":"2014-09-30 09:05:35.000000000","message":"+1 for the parameter being named cls","commit_id":"9de2d21a96f440807b9822173de3fd3b6119fbe8"}],"ironic/conductor/rpcapi.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"0a91b1c76b01ce893fd65646c33de2fc2589e37d","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        self.client \u003d rpc.get_client(target,"},{"line_number":76,"context_line":"                                     version_cap\u003dself.RPC_API_VERSION,"},{"line_number":77,"context_line":"                                     serializer\u003dserializer)"},{"line_number":78,"context_line":"        # NOTE(deva): this is going to be buggy"},{"line_number":79,"context_line":"        self.ring_manager \u003d hash_ring.HashRingManager()"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def get_topic_for(self, node):"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_4aa617c3","line":78,"updated":"2014-09-29 09:29:58.000000000","message":":( can we expand that comment? why?","commit_id":"7b29280ce46ec583d19b0131e9caa66341e954e7"}],"ironic/tests/base.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"839989d4c63f166fd8bc27ed3cadca07412262e0","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        self.addCleanup(self._restore_obj_registry)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"        self.addCleanup(self._clear_attrs)"},{"line_number":182,"context_line":"        self.addCleanup(self._reset_hash_ring_manager)"},{"line_number":183,"context_line":"        self.useFixture(fixtures.EnvironmentVariable(\u0027http_proxy\u0027))"},{"line_number":184,"context_line":"        self.policy \u003d self.useFixture(policy_fixture.PolicyFixture())"},{"line_number":185,"context_line":"        CONF.set_override(\u0027fatal_exception_format_errors\u0027, True)"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_f4d709ec","line":182,"updated":"2014-09-12 07:58:23.000000000","message":"nit:\n\n self.addCleanup(hash_ring.HashRingManager().reset)","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":1994,"name":"Zhongyue Luo","email":"zhongyue.luo@gmail.com","username":"zyluo"},"change_message_id":"eb1877a435edfd43f324c83deaf9e6199deb2d7a","unresolved":false,"context_lines":[{"line_number":195,"context_line":"            del self.__dict__[key]"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    def _reset_hash_ring_manager(self):"},{"line_number":198,"context_line":"        hash_ring.HashRingManager().reset()"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def config(self, **kw):"},{"line_number":201,"context_line":"        \"\"\"Override config options for a test.\"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_cfda6e77","line":198,"updated":"2014-09-12 08:27:26.000000000","message":"Or why not just add this line after 195 and delete 182?","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"6c8cfd451a4a3a894ee745a36e12a4e6adf69373","unresolved":false,"context_lines":[{"line_number":206,"context_line":"    def path_get(self, project_file\u003dNone):"},{"line_number":207,"context_line":"        \"\"\"Get the absolute path to a file. Used for testing the API."},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"        :param project_file: File whose path to return. Default: None."},{"line_number":210,"context_line":"        :returns: path to the specified file, or path to project root."},{"line_number":211,"context_line":"        \"\"\""},{"line_number":212,"context_line":"        root \u003d os.path.abspath(os.path.join(os.path.dirname(__file__),"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa98f980_0a8d44d3","line":209,"updated":"2014-09-12 09:35:25.000000000","message":"Would be good to add a test somwehre ensuring that the hash ring is being preserve after multiple instantiations of the HashRingManagerClass, something simple like:\n\n def test_hash_rings_built_only_once(self):\n     inst1 \u003d HashRingManager()\n\n     # add/load the hash rings using inst 1\n\n     inst2 \u003d HashRingManager()\n\n     self.assertEqual(inst1.hash_rings, inst2.hash_rings)","commit_id":"93169da7115b2d95980f8368e9789fedc176605a"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"fe60b80a66d3058a5d4117222f95a9e4949f9306","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        self.addCleanup(self._restore_obj_registry)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"        self.addCleanup(self._clear_attrs)"},{"line_number":182,"context_line":"        self.addCleanup(hash_ring.HashRingManager().reset)"},{"line_number":183,"context_line":"        self.useFixture(fixtures.EnvironmentVariable(\u0027http_proxy\u0027))"},{"line_number":184,"context_line":"        self.policy \u003d self.useFixture(policy_fixture.PolicyFixture())"},{"line_number":185,"context_line":"        CONF.set_override(\u0027fatal_exception_format_errors\u0027, True)"}],"source_content_type":"text/x-python","patch_set":6,"id":"da9df570_4fd3fc03","line":182,"updated":"2014-09-25 17:32:09.000000000","message":"This doesn\u0027t make sense:\n- why instantiate an object just to clear the class variable? The lock taken in reset is not shared across instances of HashRingManager.\n- because HashRingManager._hash_rings is a class variable, clearing it when one test finishes could adversely affect another test that is concurrently running","commit_id":"feeec3a34d8ce7fcd938973b7dac98d760f0a58d"}]}