)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":22,"context_line":"  insist on loaded components being modified, and it is desirable to have"},{"line_number":23,"context_line":"  the same semantic for all methods that load components. In practice it"},{"line_number":24,"context_line":"  has been necessary to use the \u0027force\u0027 flag with compose() to prevent"},{"line_number":25,"context_line":"  these errors being raised, which has the unfortunate side effect of"},{"line_number":26,"context_line":"  also disabling all other checks on component builders. A new"},{"line_number":27,"context_line":"  \u0027require_modified\u0027 parameter is added to compose() which defaults to"},{"line_number":28,"context_line":"  False but can be set to True if the previous default behaviour is"},{"line_number":29,"context_line":"  required."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"1f1a1f67_48755d63","line":26,"range":{"start_line":25,"start_character":28,"end_line":26,"end_character":55},"updated":"2017-07-18 11:33:32.000000000","message":"OMG. However, the check is still available at *compose* timing?","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":22,"context_line":"  insist on loaded components being modified, and it is desirable to have"},{"line_number":23,"context_line":"  the same semantic for all methods that load components. In practice it"},{"line_number":24,"context_line":"  has been necessary to use the \u0027force\u0027 flag with compose() to prevent"},{"line_number":25,"context_line":"  these errors being raised, which has the unfortunate side effect of"},{"line_number":26,"context_line":"  also disabling all other checks on component builders. A new"},{"line_number":27,"context_line":"  \u0027require_modified\u0027 parameter is added to compose() which defaults to"},{"line_number":28,"context_line":"  False but can be set to True if the previous default behaviour is"},{"line_number":29,"context_line":"  required."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"1f1a1f67_312a57da","line":26,"range":{"start_line":25,"start_character":28,"end_line":26,"end_character":55},"in_reply_to":"1f1a1f67_48755d63","updated":"2017-07-19 19:44:58.000000000","message":"We found when using compose() that we were having to always set force\u003dTrue because we want to be able to compose a ring.gz even when builders have not changed. We would prefer to be able to compose with unchanged builder without having to disable *all* the checks.  Worst case we would write an unchanged ring.gz file.\n\nSo in this patch I propose adding \u0027require_modified\u0027. This gives us these options:\n\n  compose() - apply all checks *except* for modified builders\n  compose(require_modified\u003dTrue) - apply all checks *and* require that builders are modified\n  compose(force\u003dTrue) - do not apply checks","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"- Change the CompositeRingBuilder compose() method to NOT by default"},{"line_number":20,"context_line":"  raise a ValueError if component builders have not been modified since"},{"line_number":21,"context_line":"  last loaded. With the rebalance method being added it makes less sense"},{"line_number":22,"context_line":"  insist on loaded components being modified, and it is desirable to have"},{"line_number":23,"context_line":"  the same semantic for all methods that load components. Previously it"},{"line_number":24,"context_line":"  has been necessary to use the \u0027force\u0027 flag with compose() to prevent"},{"line_number":25,"context_line":"  these errors being raised, which has the unfortunate side effect of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1f1a1f67_3a8e79d0","line":22,"range":{"start_line":21,"start_character":15,"end_line":22,"end_character":44},"updated":"2017-07-20 20:54:12.000000000","message":"How so? If the rebalance didn\u0027t actually move anything... it seems reasonable to me to refuse to write a new ring... and ValueError may as well be the way we indicate that.\n\nNow, for load_components, it seems perfectly reasonable to have require_modified be an option...","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"571da30a1c9d7dd444b50c2570309abb72768455","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"- Change the CompositeRingBuilder compose() method to NOT by default"},{"line_number":20,"context_line":"  raise a ValueError if component builders have not been modified since"},{"line_number":21,"context_line":"  last loaded. With the rebalance method being added it makes less sense"},{"line_number":22,"context_line":"  insist on loaded components being modified, and it is desirable to have"},{"line_number":23,"context_line":"  the same semantic for all methods that load components. Previously it"},{"line_number":24,"context_line":"  has been necessary to use the \u0027force\u0027 flag with compose() to prevent"},{"line_number":25,"context_line":"  these errors being raised, which has the unfortunate side effect of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1f1a1f67_bd38433d","line":22,"range":{"start_line":21,"start_character":15,"end_line":22,"end_character":44},"in_reply_to":"1f1a1f67_3a8e79d0","updated":"2017-07-20 21:13:04.000000000","message":"OK, digging around and reading some of the other comments, I think I\u0027m getting this a bit more now...\n\nUltimately, we\u0027ll want some checks in the CLI tool, like we do at https://github.com/openstack/swift/blob/2.14.0/swift/cli/ringbuilder.py#L869-L888\n\n...but meanwhile,\n\n* we need a way to elicit the \"you clearly got some crossed wires, \u0027cause this doesn\u0027t make sense\" message without having to think about the \"nothing changed\" message and\n* the existing RingBuilder.get_ring / RingData.save mechanics don\u0027t have any comparable limitations\n\nso get rid of it (by default). Maybe would have been better as a separate patch, but I can get on board.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"- Change the CompositeRingBuilder compose() method to NOT by default"},{"line_number":20,"context_line":"  raise a ValueError if component builders have not been modified since"},{"line_number":21,"context_line":"  last loaded. With the rebalance method being added it makes less sense"},{"line_number":22,"context_line":"  insist on loaded components being modified, and it is desirable to have"},{"line_number":23,"context_line":"  the same semantic for all methods that load components. Previously it"},{"line_number":24,"context_line":"  has been necessary to use the \u0027force\u0027 flag with compose() to prevent"},{"line_number":25,"context_line":"  these errors being raised, which has the unfortunate side effect of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"ff346bd7_61500034","line":22,"range":{"start_line":21,"start_character":15,"end_line":22,"end_character":44},"in_reply_to":"1f1a1f67_bd38433d","updated":"2017-07-21 21:14:43.000000000","message":"Fair point. The force behaviour is IMO a mistake (my mistake!) and I\u0027d love to get it straightened out. This seemed like a good opportunity, before we have a release that includes the current behaviour.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"}],"swift/common/ring/builder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    def _can_part_move(self, part):"},{"line_number":158,"context_line":"        return (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":159,"context_line":"                not self._has_part_moved(part))"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    @contextmanager"},{"line_number":162,"context_line":"    def debug(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_eebfcccd","side":"PARENT","line":159,"updated":"2017-06-23 20:38:44.000000000","message":"double-und it so you can\n\n    return any(b.__can_part_move() for b in cobuilders + self)\n\nmeh?","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ef4549221e4f76fd0de89e836d987f2d03922185","unresolved":false,"context_lines":[{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    def _can_part_move(self, part):"},{"line_number":158,"context_line":"        return (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":159,"context_line":"                not self._has_part_moved(part))"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    @contextmanager"},{"line_number":162,"context_line":"    def debug(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_ed0c57ed","side":"PARENT","line":159,"in_reply_to":"5f201791_eebfcccd","updated":"2017-06-27 14:35:22.000000000","message":"I like that, but all() rather than any() - you have also used any() in comment at line 1065 - am I missing something?","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":459,"context_line":"        below 1% or doesn\u0027t change by more than 1% (only happens with a ring"},{"line_number":460,"context_line":"        that can\u0027t be balanced no matter what)."},{"line_number":461,"context_line":""},{"line_number":462,"context_line":"        :param seed: a value for the random seed (optional)"},{"line_number":463,"context_line":"        :param cobuilders: a list of other builders whose _last_part_moves"},{"line_number":464,"context_line":"            state will be taken into consideration while selecting parts to"},{"line_number":465,"context_line":"            move during this rebalance (optional)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_eeec6c2a","line":462,"updated":"2017-06-23 20:38:44.000000000","message":"lol, thanks","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":462,"context_line":"        :param seed: a value for the random seed (optional)"},{"line_number":463,"context_line":"        :param cobuilders: a list of other builders whose _last_part_moves"},{"line_number":464,"context_line":"            state will be taken into consideration while selecting parts to"},{"line_number":465,"context_line":"            move during this rebalance (optional)"},{"line_number":466,"context_line":"        :returns: (number_of_partitions_altered, resulting_balance,"},{"line_number":467,"context_line":"                   number_of_removed_devices)"},{"line_number":468,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_ae44341c","line":465,"updated":"2017-06-23 20:38:44.000000000","message":"i love this name... but the spelling annoys me for some reason?  like it should be hyphenated?\n\nit\u0027s like I always call that script \u0027remaker-ings\u0027","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ef4549221e4f76fd0de89e836d987f2d03922185","unresolved":false,"context_lines":[{"line_number":462,"context_line":"        :param seed: a value for the random seed (optional)"},{"line_number":463,"context_line":"        :param cobuilders: a list of other builders whose _last_part_moves"},{"line_number":464,"context_line":"            state will be taken into consideration while selecting parts to"},{"line_number":465,"context_line":"            move during this rebalance (optional)"},{"line_number":466,"context_line":"        :returns: (number_of_partitions_altered, resulting_balance,"},{"line_number":467,"context_line":"                   number_of_removed_devices)"},{"line_number":468,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_a0be45b1","line":465,"in_reply_to":"5f201791_ae44341c","updated":"2017-06-27 14:35:22.000000000","message":"When writing it in comments I like co-builders but I\u0027m 50/50 about co_builders as the var name (something about the underscore, I want it to be a hyphen :)","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":910,"context_line":"        current time. The builder won\u0027t move a partition that has been moved"},{"line_number":911,"context_line":"        more recently than min_part_hours."},{"line_number":912,"context_line":"        \"\"\""},{"line_number":913,"context_line":"        self._part_moved_bitmap \u003d bytearray(max(2 ** (self.part_power - 3), 1))"},{"line_number":914,"context_line":"        elapsed_hours \u003d int(time() - self._last_part_moves_epoch) / 3600"},{"line_number":915,"context_line":"        if elapsed_hours \u003c\u003d 0:"},{"line_number":916,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_2e5024d4","line":913,"updated":"2017-06-23 20:38:44.000000000","message":"yup, sure","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":1062,"context_line":"                # has more than one replica of a part assigned to it - which"},{"line_number":1063,"context_line":"                # would have only been possible on rings built with an older"},{"line_number":1064,"context_line":"                # version of the code"},{"line_number":1065,"context_line":"                if (not self._can_part_move(part, cobuilders) and"},{"line_number":1066,"context_line":"                        not replicas_at_tier[dev[\u0027tiers\u0027][-1]] \u003e 1):"},{"line_number":1067,"context_line":"                    continue"},{"line_number":1068,"context_line":"                dev[\u0027parts_wanted\u0027] +\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_2eef04b1","line":1065,"updated":"2017-06-23 20:38:44.000000000","message":"I wonder if maybe replacing self._can_part_move with a closure is better than all this plumbing... or just assigning cobuilders as an attribute?\n\nsubclass?\n\n    class MultiCooperativeRingBuilder(RingBuilder):\n\n        def __init__(self, *args, **kwargs):\n            self.cobuilders \u003d kwargs.pop(\u0027cobuilders\u0027)\n            super(MultiCooperativeRingBuilder, self).__init__(*args, **kwargs)\n\n        def _can_part_moves(self, part):\n            checks \u003d [b._can_part_moves for b in self.cobuilders]\n            checks.append(super(MultiCooperativeRingBuilder, self)._can_part_moves)\n            return any(check(part) for check in checks)","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ef4549221e4f76fd0de89e836d987f2d03922185","unresolved":false,"context_lines":[{"line_number":1062,"context_line":"                # has more than one replica of a part assigned to it - which"},{"line_number":1063,"context_line":"                # would have only been possible on rings built with an older"},{"line_number":1064,"context_line":"                # version of the code"},{"line_number":1065,"context_line":"                if (not self._can_part_move(part, cobuilders) and"},{"line_number":1066,"context_line":"                        not replicas_at_tier[dev[\u0027tiers\u0027][-1]] \u003e 1):"},{"line_number":1067,"context_line":"                    continue"},{"line_number":1068,"context_line":"                dev[\u0027parts_wanted\u0027] +\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_0d06731e","line":1065,"in_reply_to":"5f201791_2eef04b1","updated":"2017-06-27 14:35:22.000000000","message":"yep, excessive plumbing invariably annoys me. I considered a closure like solution, but a voice in my head kept saying \u0027keep it simple\u0027, for first cut at least. If we\u0027re happy this works then I will certainly reconsider. But I\u0027m not sure I want cobuilders to be a property of the instance that outlives the rebalance operation - need to be careful not to end up in a loop when one builder calls a cobuilder._can_part_move and the cobuilder has cobuilders...\n\nSubclassing feels like it will become onerous - we need to load all the builder before we have cobuilders, so would we then reinstantiate using the subclass, or populate the mutable cobuilders retrospectively (sounds scarey). Or have I misunderstood?\n\nPerhaps assigning cobuilders as an attr and then wrapping rebalance in a try/finally to ensure it is always unset?","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"fc99d265d2c410a5966d13458bd5efdc22e5744a","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        # _last_part_moves_epoch indicates the time the offsets in"},{"line_number":117,"context_line":"        # _last_part_moves is based on."},{"line_number":118,"context_line":"        self._last_part_moves_epoch \u003d 0"},{"line_number":119,"context_line":"        self._cobuilders \u003d {self}"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"        self._last_part_gather_start \u003d 0"},{"line_number":122,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f201791_f58ce236","line":119,"updated":"2017-06-27 22:00:52.000000000","message":"heh... i\u0027m my own grandpa?","commit_id":"7a45eec38223345c3b8d4f041cad86abe4cc9608"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"fc99d265d2c410a5966d13458bd5efdc22e5744a","unresolved":false,"context_lines":[{"line_number":550,"context_line":"        try:"},{"line_number":551,"context_line":"            return self._rebalance(seed)"},{"line_number":552,"context_line":"        finally:"},{"line_number":553,"context_line":"            self._cobuilders \u003d {self}"},{"line_number":554,"context_line":""},{"line_number":555,"context_line":"    def _build_dispersion_graph(self, old_replica2part2dev\u003dNone):"},{"line_number":556,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f201791_b5b0aade","line":553,"updated":"2017-06-27 22:00:52.000000000","message":"ok, maybe.","commit_id":"7a45eec38223345c3b8d4f041cad86abe4cc9608"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0909bd18adf22e5fd1cbf253470feb65033da137","unresolved":false,"context_lines":[{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"        if not hasattr(builder, \u0027devs\u0027):"},{"line_number":1677,"context_line":"            builder_dict \u003d builder"},{"line_number":1678,"context_line":"            builder \u003d cls(1, 1, 1)"},{"line_number":1679,"context_line":"            builder.copy_from(builder_dict)"},{"line_number":1680,"context_line":""},{"line_number":1681,"context_line":"        if not hasattr(builder, \u0027_id\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_359d900a","line":1678,"updated":"2017-06-28 23:17:46.000000000","message":"zomg, that\u0027s some OO business right there!","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":158,"context_line":"        # if min_part_hours is zero then checking _last_part_moves will not"},{"line_number":159,"context_line":"        # indicate if the part has already moved during the current rebalance,"},{"line_number":160,"context_line":"        # but _has_part_moved will."},{"line_number":161,"context_line":"        return (not self._last_part_moves or"},{"line_number":162,"context_line":"                (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":163,"context_line":"                 not self._has_part_moved(part)))"},{"line_number":164,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_8210f6a9","line":161,"updated":"2017-07-18 11:33:32.000000000","message":"I\u0027m not entirely sure on this change. That is because if not _last_part_moves assigned the _can_part_moves always return True even if _has_part_moved(part) is True.\n\nBasically, _last_part_moves should be initialized at the beginning of the rebalance work... so perhaps, something like making a property could be safe?\n\n@property\ndef _last_part_moves(self):\n   if not hasattr(self, \u0027__last_part_moves\u0027):\n     self.__last_part_moves \u003d self._last_part_moves \u003d array(\u0027B\u0027, itertools.repeat(0, self.parts))\n   return self.__last_part_moves\n\n\nCurrently, _can_part_moves is called only in sort of partition gathering so the code change is safe right now but it\u0027s also unnecessary because self._last_part_moves should be initiated at  [1].\n\nAm i missing something?\n\n1: https://github.com/openstack/swift/blob/master/swift/common/ring/builder.py#L486\n\n(p.s. after looking at the composite ring builder change)\n\nIt seems like the case we can hit _last_part_moves \u003d None is only non-rebalance case, right? because once the ring rebalanced, the _last_part_moves array should be saved in the builder file and it can be loaded when CompositeRingBuilder loads the component rings.\n\nFWIW, this change makes my test[2] to pass my test but I don\u0027t know if it\u0027s expected or not for you.\n\n\nOR, I\u0027m reallizing why not we can make the _last_part_moves as 0 filled array at __init__ rather than assigning None there...\n\n2: https://gist.github.com/bloodeagle40234/ebbbb76b63ccf446380ff82732ccc643","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":158,"context_line":"        # if min_part_hours is zero then checking _last_part_moves will not"},{"line_number":159,"context_line":"        # indicate if the part has already moved during the current rebalance,"},{"line_number":160,"context_line":"        # but _has_part_moved will."},{"line_number":161,"context_line":"        return (not self._last_part_moves or"},{"line_number":162,"context_line":"                (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":163,"context_line":"                 not self._has_part_moved(part)))"},{"line_number":164,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_b948775c","line":161,"in_reply_to":"1f1a1f67_44362e53","updated":"2017-07-20 07:30:53.000000000","message":"Yes, the test [2] passed correctly at least as well as I wrote. (I didn\u0027t check the ring status after rebalance/compose yet though)\n\nAnd on your scenario, it looks safe to me too.\n\nProbably, the reason I didn\u0027t like to change native ring builder so much is because I\u0027m digging the logic around this _can_part_move with [1] that is trying to improve rebalance work for a (or several?) dead lock case(s). Though that is still under consideration but i think we need to re-think about which part can be moved for the case(s).\n\nTo avoid RingBuilder change as possible, I wrote a sample at [2].\n\nI think the situation that _can_part_move called before rebalance is only a case in CooperativeRingBuilder/CompositeRingBuilder so that we can move the code over there, maybe?\n\n1: https://review.openstack.org/#/c/479223/\n2: https://review.openstack.org/#/c/485469/","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":158,"context_line":"        # if min_part_hours is zero then checking _last_part_moves will not"},{"line_number":159,"context_line":"        # indicate if the part has already moved during the current rebalance,"},{"line_number":160,"context_line":"        # but _has_part_moved will."},{"line_number":161,"context_line":"        return (not self._last_part_moves or"},{"line_number":162,"context_line":"                (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":163,"context_line":"                 not self._has_part_moved(part)))"},{"line_number":164,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_44362e53","line":161,"in_reply_to":"1f1a1f67_8210f6a9","updated":"2017-07-19 19:44:58.000000000","message":"I didnt initialize _last_part_moves because it is used as a condition for a debug message at [1] so needs to be None when rebalance is first called. I would need to add another attr to track new rebalance if I change _last_part_moves to be initialized earlier.\n\nWith this patch we can now call _can_part_move *before* the builder has been rebalanced because we may call it from another co-builder rebalance method. So I need the check for _last_part_move.\n\nThe logic in this method is therefore:\n\n  if not self._last_part_moves -\u003e builder has not been rebalance, ok to move the part\n  else check _last_part_moves[part] \u003e\u003d self.min_part_hours etc\n\nYour test at  https://gist.github.com/bloodeagle40234/ebbbb76b63ccf446380ff82732ccc643 passes with the patch as it is, correct?\n\n\n\n[1] https://github.com/openstack/swift/blob/f7e5b1f44f6e4392f31e6f15263e16b4538d696c/swift/common/ring/builder.py#L485-L487\n[2]  https://gist.github.com/bloodeagle40234/ebbbb76b63ccf446380ff82732ccc643","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":902,"context_line":"        \"\"\""},{"line_number":903,"context_line":"        self._part_moved_bitmap \u003d bytearray(max(2 ** (self.part_power - 3), 1))"},{"line_number":904,"context_line":"        elapsed_hours \u003d int(time() - self._last_part_moves_epoch) / 3600"},{"line_number":905,"context_line":"        if elapsed_hours \u003c\u003d 0 or not self._last_part_moves:"},{"line_number":906,"context_line":"            return"},{"line_number":907,"context_line":"        for part in range(self.parts):"},{"line_number":908,"context_line":"            # The \"min(self._last_part_moves[part] + elapsed_hours, 0xff)\""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_87a2df4a","line":905,"range":{"start_line":905,"start_character":33,"end_line":905,"end_character":58},"updated":"2017-07-20 20:54:12.000000000","message":"Is this ever true? Didn\u0027t we *just* make sure self._last_part_moves isn\u0027t None before calling _update_last_part_moves?\n\nOh, I get it... over in CooperativeRingBuilder we reach down into this \"private\" method, and this builder may not have called rebalance yet... hmm...\n\nWhy don\u0027t we just move the _last_part_moves initialization here, like we did the _part_moved_bitmap initialization?","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":902,"context_line":"        \"\"\""},{"line_number":903,"context_line":"        self._part_moved_bitmap \u003d bytearray(max(2 ** (self.part_power - 3), 1))"},{"line_number":904,"context_line":"        elapsed_hours \u003d int(time() - self._last_part_moves_epoch) / 3600"},{"line_number":905,"context_line":"        if elapsed_hours \u003c\u003d 0 or not self._last_part_moves:"},{"line_number":906,"context_line":"            return"},{"line_number":907,"context_line":"        for part in range(self.parts):"},{"line_number":908,"context_line":"            # The \"min(self._last_part_moves[part] + elapsed_hours, 0xff)\""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_1b4ac77f","line":905,"range":{"start_line":905,"start_character":33,"end_line":905,"end_character":58},"in_reply_to":"1f1a1f67_87a2df4a","updated":"2017-07-21 21:14:43.000000000","message":"_last_part_moves\u003d\u003dNone is used as a condition for a debug log on first rebalance so I don\u0027t want to set it here which may now be called before first rebalance","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d0139ca9c27e54935d7475df0e6cc415d1e693fc","unresolved":false,"context_lines":[{"line_number":159,"context_line":"        # if min_part_hours is zero then checking _last_part_moves will not"},{"line_number":160,"context_line":"        # indicate if the part has already moved during the current rebalance,"},{"line_number":161,"context_line":"        # but _has_part_moved will."},{"line_number":162,"context_line":"        return (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":163,"context_line":"                not self._has_part_moved(part))"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    @contextmanager"}],"source_content_type":"text/x-python","patch_set":8,"id":"ff346bd7_4113bc67","line":162,"range":{"start_line":162,"start_character":8,"end_line":162,"end_character":43},"updated":"2017-07-21 21:26:22.000000000","message":"@Kota no change here - it is now in the subclass :)","commit_id":"4eb6a633f7fb10dc032b226389787dfb26a2a9c5"}],"swift/common/ring/composite_builder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":558,"context_line":"        # TODO: we may want to update the composite builder version, so we"},{"line_number":559,"context_line":"        # should probably check that these builder files are same as any"},{"line_number":560,"context_line":"        # component metadata we may have?"},{"line_number":561,"context_line":"        builders \u003d [RingBuilder.load(f) for f in builder_files]"},{"line_number":562,"context_line":"        # don\u0027t let the same builder go first each time"},{"line_number":563,"context_line":"        random.shuffle(builders)"},{"line_number":564,"context_line":"        total_moved \u003d 0"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_8e9858c0","line":561,"updated":"2017-06-23 20:38:44.000000000","message":"here we load... who/where is the save?","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ef4549221e4f76fd0de89e836d987f2d03922185","unresolved":false,"context_lines":[{"line_number":558,"context_line":"        # TODO: we may want to update the composite builder version, so we"},{"line_number":559,"context_line":"        # should probably check that these builder files are same as any"},{"line_number":560,"context_line":"        # component metadata we may have?"},{"line_number":561,"context_line":"        builders \u003d [RingBuilder.load(f) for f in builder_files]"},{"line_number":562,"context_line":"        # don\u0027t let the same builder go first each time"},{"line_number":563,"context_line":"        random.shuffle(builders)"},{"line_number":564,"context_line":"        total_moved \u003d 0"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_16341549","line":561,"in_reply_to":"5f201791_8e9858c0","updated":"2017-06-27 14:35:22.000000000","message":"will fix. Although I am not sure if we prefer to do the component builder saves automatically here or give the caller chance to validate before saving?","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"afcd4566fa31b02a3cfe496eda873e409f957b2a","unresolved":false,"context_lines":[{"line_number":565,"context_line":"        for builder in builders:"},{"line_number":566,"context_line":"            # it is safe to include this builder in cobuilders, it is ignored"},{"line_number":567,"context_line":"            # TODO: error handling"},{"line_number":568,"context_line":"            # TODO: is it useful to return more than total_moved?"},{"line_number":569,"context_line":"            moved, _, _ \u003d builder.rebalance(cobuilders\u003dbuilders)"},{"line_number":570,"context_line":"            total_moved +\u003d moved"},{"line_number":571,"context_line":"        return total_moved"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_ee87cc60","line":568,"updated":"2017-06-23 20:38:44.000000000","message":"caller doesn\u0027t have access in-memory builders:?","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ef4549221e4f76fd0de89e836d987f2d03922185","unresolved":false,"context_lines":[{"line_number":565,"context_line":"        for builder in builders:"},{"line_number":566,"context_line":"            # it is safe to include this builder in cobuilders, it is ignored"},{"line_number":567,"context_line":"            # TODO: error handling"},{"line_number":568,"context_line":"            # TODO: is it useful to return more than total_moved?"},{"line_number":569,"context_line":"            moved, _, _ \u003d builder.rebalance(cobuilders\u003dbuilders)"},{"line_number":570,"context_line":"            total_moved +\u003d moved"},{"line_number":571,"context_line":"        return total_moved"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f201791_ada41fa7","line":568,"in_reply_to":"5f201791_ee87cc60","updated":"2017-06-27 14:35:22.000000000","message":"not as things stand. We need some more work if we want to make the builders be an instance property that persists between rebalance and compose for example. But, that assumes the caller will not change and save builders once it has passed their filenames to CompositeRingBuilder.","commit_id":"90dad816c076503b6c0aa3503edd2660b0f3b358"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"786999e66d46d449b200c8b1836ce33882a37b8b","unresolved":false,"context_lines":[{"line_number":506,"context_line":"    def _load_components(self, builder_files\u003dNone, force\u003dFalse):"},{"line_number":507,"context_line":"        builder_files \u003d builder_files or self._builder_files"},{"line_number":508,"context_line":"        builders \u003d [CooperativeRingBuilder.load(f) for f in builder_files]"},{"line_number":509,"context_line":"        cobuilders \u003d set()"},{"line_number":510,"context_line":"        [b.update_cobuilders(cobuilders) for b in builders]"},{"line_number":511,"context_line":"        check_builder_ids(builders)"},{"line_number":512,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_175bbdf5","line":509,"range":{"start_line":509,"start_character":21,"end_line":509,"end_character":26},"updated":"2017-06-28 20:08:13.000000000","message":"set gets updated as we go","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0909bd18adf22e5fd1cbf253470feb65033da137","unresolved":false,"context_lines":[{"line_number":507,"context_line":"        builder_files \u003d builder_files or self._builder_files"},{"line_number":508,"context_line":"        builders \u003d [CooperativeRingBuilder.load(f) for f in builder_files]"},{"line_number":509,"context_line":"        cobuilders \u003d set()"},{"line_number":510,"context_line":"        [b.update_cobuilders(cobuilders) for b in builders]"},{"line_number":511,"context_line":"        check_builder_ids(builders)"},{"line_number":512,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":513,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_d86e9195","line":510,"updated":"2017-06-28 23:17:46.000000000","message":"yeah I could *not* see what was going on here until I had this open in my editor.","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"786999e66d46d449b200c8b1836ce33882a37b8b","unresolved":false,"context_lines":[{"line_number":572,"context_line":"        with the existing composition of builders, unless the optional"},{"line_number":573,"context_line":"        ``force`` flag if set True."},{"line_number":574,"context_line":""},{"line_number":575,"context_line":"        :param builder_files: Optional list of paths to ring builder"},{"line_number":576,"context_line":"            files that will be used to load the component ring builders."},{"line_number":577,"context_line":"            Typically the list of component builder files will have been set"},{"line_number":578,"context_line":"            when the instance was constructed, for example when using the"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_974d8d3c","line":575,"updated":"2017-06-28 20:08:13.000000000","message":"plan to remove this and make load_components public","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"786999e66d46d449b200c8b1836ce33882a37b8b","unresolved":false,"context_lines":[{"line_number":612,"context_line":"                    (builder_file, err))"},{"line_number":613,"context_line":"        # TODO: should we save components as we go or only save if all succeed?"},{"line_number":614,"context_line":"        for builder_file, builder in zip(builder_files, builders):"},{"line_number":615,"context_line":"            builder.save(builder_file)"},{"line_number":616,"context_line":"        # return results in component order"},{"line_number":617,"context_line":"        return [results[builder] for builder in builders]"},{"line_number":618,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_d747851c","line":615,"updated":"2017-06-28 20:08:13.000000000","message":"add validate()","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"786999e66d46d449b200c8b1836ce33882a37b8b","unresolved":false,"context_lines":[{"line_number":623,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":624,"context_line":"        self._cobuilders \u003d {self}"},{"line_number":625,"context_line":""},{"line_number":626,"context_line":"    def update_cobuilders(self, cobuilders):"},{"line_number":627,"context_line":"        \"\"\""},{"line_number":628,"context_line":"        Updates the given set of cobuilders with a reference to this builder"},{"line_number":629,"context_line":"        and install the set of cobuilders in this builder. Note that the set of"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f201791_373fd987","line":626,"updated":"2017-06-28 20:08:13.000000000","message":"could be smarter, but this will do for now","commit_id":"3fb1b30a2ca7fba8e0a2edc1dd881e8543f98788"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9db31e194bf26c46dce62e3241dc3783efc8b5cb","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"        cobuilders \u003d set()"},{"line_number":517,"context_line":"        builders \u003d [CooperativeRingBuilder.load(f, cobuilders\u003dcobuilders)"},{"line_number":518,"context_line":"                    for f in builder_files]"},{"line_number":519,"context_line":"        check_builder_ids(builders)"},{"line_number":520,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":521,"context_line":"        if self.components and self._builder_files and not ignore_errors:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_3119e9d6","line":518,"updated":"2017-06-30 14:38:54.000000000","message":"I\u0027m not sure if passing in a ref to CompositeRingBuilder is needed, but there\u0027s a cognitive dissidence (for me) caused by the distance from this line (which passes in an empty set) ...","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"cf925a6c2047c71dd5349ac2e1a5623f25424bb5","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"        cobuilders \u003d set()"},{"line_number":517,"context_line":"        builders \u003d [CooperativeRingBuilder.load(f, cobuilders\u003dcobuilders)"},{"line_number":518,"context_line":"                    for f in builder_files]"},{"line_number":519,"context_line":"        check_builder_ids(builders)"},{"line_number":520,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":521,"context_line":"        if self.components and self._builder_files and not ignore_errors:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_25907c4e","line":518,"in_reply_to":"3f1d235d_3119e9d6","updated":"2017-06-30 20:50:46.000000000","message":"yeah, I knew I needed fresh eyes / someone to beat it into me.","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9db31e194bf26c46dce62e3241dc3783efc8b5cb","unresolved":false,"context_lines":[{"line_number":525,"context_line":"                    \"None of the component builders has been modified\""},{"line_number":526,"context_line":"                    \" since the existing composite ring was built.\")"},{"line_number":527,"context_line":"        self._set_builder_files(builder_files)"},{"line_number":528,"context_line":"        self._builders \u003d builders"},{"line_number":529,"context_line":"        return self._builder_files, self._builders"},{"line_number":530,"context_line":""},{"line_number":531,"context_line":"    def load_components(self, builder_files\u003dNone, ignore_errors\u003dFalse,"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_7173e116","line":528,"updated":"2017-06-30 14:38:54.000000000","message":"we keep a reference to the list/set of builders anyway - why not just share it (since it\u0027s obvious from *just this method* what\u0027s in it and where it came from)","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8953afc9660abc435202bf157dc9626fdbb4c144","unresolved":false,"context_lines":[{"line_number":597,"context_line":"            composite ring, or if there has been no change with respect to the"},{"line_number":598,"context_line":"            existing ring."},{"line_number":599,"context_line":"        \"\"\""},{"line_number":600,"context_line":"        self.load_components(builder_files, ignore_errors\u003dforce,"},{"line_number":601,"context_line":"                             require_modified\u003dnot force)"},{"line_number":602,"context_line":"        self.ring_data \u003d compose_rings(self._builders)"},{"line_number":603,"context_line":"        self.version +\u003d 1"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_0c2efda4","line":600,"range":{"start_line":600,"start_character":13,"end_line":600,"end_character":28},"updated":"2017-06-29 14:44:25.000000000","message":"yes, load_components rather than _load_components to preserve the current semantic of compose()","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8953afc9660abc435202bf157dc9626fdbb4c144","unresolved":false,"context_lines":[{"line_number":647,"context_line":""},{"line_number":648,"context_line":""},{"line_number":649,"context_line":"class CooperativeRingBuilder(RingBuilder):"},{"line_number":650,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":651,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":652,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":653,"context_line":"        self._cobuilders \u003d cobuilders if cobuilders is not None else set()"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_af037b0d","line":650,"range":{"start_line":650,"start_character":61,"end_line":650,"end_character":71},"updated":"2017-06-29 14:44:25.000000000","message":"it\u0027s easy enough to see how this can become a ref to a CompositeRingBuilder if that is preferred","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9db31e194bf26c46dce62e3241dc3783efc8b5cb","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":652,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":653,"context_line":"        self._cobuilders \u003d cobuilders if cobuilders is not None else set()"},{"line_number":654,"context_line":"        self._cobuilders.add(self)"},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"    def _can_part_move(self, part):"},{"line_number":657,"context_line":"        return all(super(CooperativeRingBuilder, b)._can_part_move(part)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_317e89e4","line":654,"updated":"2017-06-30 14:38:54.000000000","message":"... and *this* line which adds it self as the \"one and only member\" to the *passed in* reference.\n\nIt\u0027s like... hey, this doesn\u0027t belong to us, it came from somewhere else - why are *we* mutating someone else\u0027s state?  Encapsulation - that\u0027s a word that has something to do with OO something something right?","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"Under the hood, the composite ring has a ``_replica2part2dev_id`` table that is"},{"line_number":67,"context_line":"the union of the tables from the component rings. Whenever the component rings"},{"line_number":68,"context_line":"are rebalanced, the composite ring must be rebuilt. There is no dynamic"},{"line_number":69,"context_line":"rebuilding of the composite ring."},{"line_number":70,"context_line":""},{"line_number":71,"context_line":".. note::"},{"line_number":72,"context_line":"    The order in which component rings are combined into a composite ring is"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_9306061c","line":69,"range":{"start_line":68,"start_character":51,"end_line":69,"end_character":33},"updated":"2017-07-18 11:33:32.000000000","message":"Obviously, this change need to change this block, in particular the yellow sentence.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"Under the hood, the composite ring has a ``_replica2part2dev_id`` table that is"},{"line_number":67,"context_line":"the union of the tables from the component rings. Whenever the component rings"},{"line_number":68,"context_line":"are rebalanced, the composite ring must be rebuilt. There is no dynamic"},{"line_number":69,"context_line":"rebuilding of the composite ring."},{"line_number":70,"context_line":""},{"line_number":71,"context_line":".. note::"},{"line_number":72,"context_line":"    The order in which component rings are combined into a composite ring is"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_6755acf9","line":69,"range":{"start_line":68,"start_character":51,"end_line":69,"end_character":33},"in_reply_to":"1f1a1f67_9306061c","updated":"2017-07-19 19:44:58.000000000","message":"I think this is still correct - rebalancing the component rings does not cause the composite ring to be updated/recomposed. But I do need to add to the docstring for CompositeRingBuilder.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5a1d68f796e4247325f96289e342675f2afb359d","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"        builders \u003d []"},{"line_number":517,"context_line":"        [builders.append(CooperativeRingBuilder.load(f, cobuilders\u003dbuilders))"},{"line_number":518,"context_line":"         for f in builder_files]"},{"line_number":519,"context_line":"        check_builder_ids(builders)"},{"line_number":520,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":521,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f1d235d_b6d60027","line":518,"updated":"2017-07-01 00:25:10.000000000","message":"I\u0027d have preferred to unroll this\n\n    diff --git a/swift/common/ring/composite_builder.py b/swift/common/ring/composite_builder.py\n    index 5c62b9f..80eee05 100644\n    --- a/swift/common/ring/composite_builder.py\n    +++ b/swift/common/ring/composite_builder.py\n    @@ -514,8 +514,8 @@ class CompositeRingBuilder(object):\n                 raise ValueError(\u0027Two or more component builders are required.\u0027)\n     \n             builders \u003d []\n    -        [builders.append(CooperativeRingBuilder.load(f, cobuilders\u003dbuilders))\n    -         for f in builder_files]\n    +        for f in builder_files:\n    +            builders.append(CooperativeRingBuilder.load(f, cobuilders\u003dbuilders))\n             check_builder_ids(builders)\n             new_metadata \u003d _make_composite_metadata(builders)\n             if self.components and self._builder_files and not force:","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"        builders \u003d []"},{"line_number":517,"context_line":"        [builders.append(CooperativeRingBuilder.load(f, cobuilders\u003dbuilders))"},{"line_number":518,"context_line":"         for f in builder_files]"},{"line_number":519,"context_line":"        check_builder_ids(builders)"},{"line_number":520,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":521,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_8467a64d","line":518,"in_reply_to":"1f1a1f67_cdf17f6d","updated":"2017-07-19 19:44:58.000000000","message":"Done","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":515,"context_line":""},{"line_number":516,"context_line":"        builders \u003d []"},{"line_number":517,"context_line":"        [builders.append(CooperativeRingBuilder.load(f, cobuilders\u003dbuilders))"},{"line_number":518,"context_line":"         for f in builder_files]"},{"line_number":519,"context_line":"        check_builder_ids(builders)"},{"line_number":520,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":521,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_cdf17f6d","line":518,"in_reply_to":"3f1d235d_b6d60027","updated":"2017-07-18 11:33:32.000000000","message":"+1","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":636,"context_line":"                results[builder] \u003d {"},{"line_number":637,"context_line":"                    \u0027builder\u0027: builder,"},{"line_number":638,"context_line":"                    \u0027builder_file\u0027: builder_file,"},{"line_number":639,"context_line":"                    \u0027result\u0027: builder.rebalance()"},{"line_number":640,"context_line":"                }"},{"line_number":641,"context_line":"                builder.validate()"},{"line_number":642,"context_line":"            except RingBuilderError as err:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_93fde66f","line":639,"updated":"2017-07-18 11:33:32.000000000","message":"OIC, i finally got why the RingBuilder change needed. That is because here, co-builders may be not yet rebalanced but the current builder wants to know the status for *already rebalanced* co-builders","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":636,"context_line":"                results[builder] \u003d {"},{"line_number":637,"context_line":"                    \u0027builder\u0027: builder,"},{"line_number":638,"context_line":"                    \u0027builder_file\u0027: builder_file,"},{"line_number":639,"context_line":"                    \u0027result\u0027: builder.rebalance()"},{"line_number":640,"context_line":"                }"},{"line_number":641,"context_line":"                builder.validate()"},{"line_number":642,"context_line":"            except RingBuilderError as err:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_f99954c3","line":639,"in_reply_to":"1f1a1f67_470f5087","updated":"2017-07-20 07:30:53.000000000","message":"So thinking of that purpose, perhaps it would be better to override _can_part_move in CooperativeBuilder?","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":636,"context_line":"                results[builder] \u003d {"},{"line_number":637,"context_line":"                    \u0027builder\u0027: builder,"},{"line_number":638,"context_line":"                    \u0027builder_file\u0027: builder_file,"},{"line_number":639,"context_line":"                    \u0027result\u0027: builder.rebalance()"},{"line_number":640,"context_line":"                }"},{"line_number":641,"context_line":"                builder.validate()"},{"line_number":642,"context_line":"            except RingBuilderError as err:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_470f5087","line":639,"in_reply_to":"1f1a1f67_93fde66f","updated":"2017-07-19 19:44:58.000000000","message":"yes plus  a builder may need to query the _can_part_move of a cobuilder that has not yet been rebalanced.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":650,"context_line":"        return [results[builder] for builder in self._builders]"},{"line_number":651,"context_line":""},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"class CooperativeRingBuilder(RingBuilder):"},{"line_number":654,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":655,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":656,"context_line":"            part_power, replicas, min_part_hours)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_0df1b795","line":653,"updated":"2017-07-18 11:33:32.000000000","message":"Probably, I want a help description what\u0027s the cooperative ring builder here.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":650,"context_line":"        return [results[builder] for builder in self._builders]"},{"line_number":651,"context_line":""},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"class CooperativeRingBuilder(RingBuilder):"},{"line_number":654,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":655,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":656,"context_line":"            part_power, replicas, min_part_hours)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_877e88e1","line":653,"in_reply_to":"1f1a1f67_0df1b795","updated":"2017-07-19 19:44:58.000000000","message":"Done","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5a1d68f796e4247325f96289e342675f2afb359d","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        # will effectively be a no-op for co-builders that have already been"},{"line_number":671,"context_line":"        # updated in last hour"},{"line_number":672,"context_line":"        [super(CooperativeRingBuilder, b)._update_last_part_moves()"},{"line_number":673,"context_line":"         for b in self._cobuilders]"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f1d235d_f6eb58d9","line":673,"updated":"2017-07-01 00:25:10.000000000","message":"oh, but you\u0027re like into the side-effect having list creation today... interesting... I typically think of list comprehensions as *creating* something - if they\u0027re just *doing* something... then the simplest way to express that is a for loop, no?","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        # will effectively be a no-op for co-builders that have already been"},{"line_number":671,"context_line":"        # updated in last hour"},{"line_number":672,"context_line":"        [super(CooperativeRingBuilder, b)._update_last_part_moves()"},{"line_number":673,"context_line":"         for b in self._cobuilders]"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_8aa0cdf5","line":673,"in_reply_to":"1f1a1f67_ad68831c","updated":"2017-07-19 19:44:58.000000000","message":"Done","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        # will effectively be a no-op for co-builders that have already been"},{"line_number":671,"context_line":"        # updated in last hour"},{"line_number":672,"context_line":"        [super(CooperativeRingBuilder, b)._update_last_part_moves()"},{"line_number":673,"context_line":"         for b in self._cobuilders]"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_ad68831c","line":673,"in_reply_to":"3f1d235d_b15eb541","updated":"2017-07-18 11:33:32.000000000","message":"I support Clay\u0027s suggestion. Current code looks confusable if we may need \"return\" to be responsive for callers but actually it\u0027s NOT. it should finish without return value.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2eb85e00cf576dfc1ecff7463e0b5b01ecec0820","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        # will effectively be a no-op for co-builders that have already been"},{"line_number":671,"context_line":"        # updated in last hour"},{"line_number":672,"context_line":"        [super(CooperativeRingBuilder, b)._update_last_part_moves()"},{"line_number":673,"context_line":"         for b in self._cobuilders]"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f1d235d_b15eb541","line":673,"in_reply_to":"3f1d235d_f6eb58d9","updated":"2017-07-05 08:05:17.000000000","message":"hmmm, yeah. Looking at line 660, there was probably some now indefensible evolution of code that got me to this construct here.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":367,"context_line":"class CompositeRingBuilder(object):"},{"line_number":368,"context_line":"    \"\"\""},{"line_number":369,"context_line":"    Provides facility to create, persist, load, rebalance  and update composite"},{"line_number":370,"context_line":"    rings, for example::"},{"line_number":371,"context_line":""},{"line_number":372,"context_line":"        # create a CompositeRingBuilder instance with a list of"},{"line_number":373,"context_line":"        # component builder files"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_f7313ca4","line":370,"updated":"2017-07-20 20:54:12.000000000","message":"How much of the RingBuilder API should this ultimately take on? I could see it being useful to have a validate method that validates all the component builders, for instance.\n\nNot that we need to do it all *now*, but something to think about...","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        builders \u003d []"},{"line_number":520,"context_line":"        for builder_file in builder_files:"},{"line_number":521,"context_line":"            builders.append(CooperativeRingBuilder.load(builder_file,"},{"line_number":522,"context_line":"                                                        cobuilders\u003dbuilders))"},{"line_number":523,"context_line":"        check_builder_ids(builders)"},{"line_number":524,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":525,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_e23501aa","line":522,"range":{"start_line":522,"start_character":56,"end_line":522,"end_character":75},"updated":"2017-07-20 20:54:12.000000000","message":"This seems worth a comment. We pass the *same array reference* to all the builders so when we do the append, we\u0027re adding the newly-created builder *to all currently instantiated builders\u0027 cobuilder lists, including its own*.\n\nFeels rather involuted and kinda spooky... and it reminds me just how much trust I put in the GC :P","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        builders \u003d []"},{"line_number":520,"context_line":"        for builder_file in builder_files:"},{"line_number":521,"context_line":"            builders.append(CooperativeRingBuilder.load(builder_file,"},{"line_number":522,"context_line":"                                                        cobuilders\u003dbuilders))"},{"line_number":523,"context_line":"        check_builder_ids(builders)"},{"line_number":524,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":525,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_86fe5a03","line":522,"range":{"start_line":522,"start_character":56,"end_line":522,"end_character":75},"in_reply_to":"1f1a1f67_a0d497be","updated":"2017-07-21 21:14:43.000000000","message":"Done - applied Tim\u0027s patch, thanks Tim!","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9729023e6d0fbd3484be285da54c816b7c732f26","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        builders \u003d []"},{"line_number":520,"context_line":"        for builder_file in builder_files:"},{"line_number":521,"context_line":"            builders.append(CooperativeRingBuilder.load(builder_file,"},{"line_number":522,"context_line":"                                                        cobuilders\u003dbuilders))"},{"line_number":523,"context_line":"        check_builder_ids(builders)"},{"line_number":524,"context_line":"        new_metadata \u003d _make_composite_metadata(builders)"},{"line_number":525,"context_line":"        if self.components and self._builder_files and not force:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_a0d497be","line":522,"range":{"start_line":522,"start_character":56,"end_line":522,"end_character":75},"in_reply_to":"1f1a1f67_e23501aa","updated":"2017-07-21 07:43:44.000000000","message":"something like validation method could be worth?\n\nlikely:\n\ndef validate_cobuilders(coop_builder):\n  builder_ids \u003d set([builder.id for builder in coop_bulder._cobuilders])\n  for cobuilder in coop_builder._cobuilders:\n     cobuilder_ids \u003d set([builder.id for builder in coop_bulder._cobuilders])\n     if builder_ids !\u003d builder_ids:\n       raise ValueError(\u0027CooperativeRingBuilder has different state from cobuilders\u0027)\n\nMaybe it should be in somewhere in CooperativeRingBulider (not here, this is in CompositeRingBuilder)\n\nOh... *NO*, it is impossible because  the builders list is growing in this for-loop.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":613,"context_line":"        self.components \u003d new_metadata[\u0027components\u0027]"},{"line_number":614,"context_line":"        return self.ring_data"},{"line_number":615,"context_line":""},{"line_number":616,"context_line":"    def rebalance(self):"},{"line_number":617,"context_line":"        \"\"\""},{"line_number":618,"context_line":"        Cooperatively rebalances all component ring builders. Each builder is"},{"line_number":619,"context_line":"        rebalanced using the other components as co-builders."}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_c2e0c5cf","line":616,"updated":"2017-07-20 20:54:12.000000000","message":"nit: Not gonna take a `seed` arg? I don\u0027t really like it in the base RingBuilder, but the inconsistency seems a little weird.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":613,"context_line":"        self.components \u003d new_metadata[\u0027components\u0027]"},{"line_number":614,"context_line":"        return self.ring_data"},{"line_number":615,"context_line":""},{"line_number":616,"context_line":"    def rebalance(self):"},{"line_number":617,"context_line":"        \"\"\""},{"line_number":618,"context_line":"        Cooperatively rebalances all component ring builders. Each builder is"},{"line_number":619,"context_line":"        rebalanced using the other components as co-builders."}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_c63462e1","line":616,"in_reply_to":"1f1a1f67_c2e0c5cf","updated":"2017-07-21 21:14:43.000000000","message":"right now it would have no usage unless I wrote a test that used it :) can we add it when need arises?","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":639,"context_line":"        self._load_components()"},{"line_number":640,"context_line":"        component_builders \u003d zip(self._builder_files, self._builders)"},{"line_number":641,"context_line":"        # don\u0027t let the same builder go first each time"},{"line_number":642,"context_line":"        shuffle(component_builders)"},{"line_number":643,"context_line":"        results \u003d {}"},{"line_number":644,"context_line":"        for builder_file, builder in component_builders:"},{"line_number":645,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_22f2d99b","line":642,"updated":"2017-07-20 20:54:12.000000000","message":"Would it be better to round-robin it, based on self.version? *shrug*","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                }"},{"line_number":651,"context_line":"                builder.validate()"},{"line_number":652,"context_line":"            except RingBuilderError as err:"},{"line_number":653,"context_line":"                raise RingBuilderError("},{"line_number":654,"context_line":"                    \u0027An error occurred while rebalancing component %s: %s\u0027 %"},{"line_number":655,"context_line":"                    (builder_file, err))"},{"line_number":656,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_9aad8dec","line":653,"updated":"2017-07-20 20:54:12.000000000","message":"Should we add a\n\n self._builders \u003d None\n\nbefore raising? I\u0027m thinking someone may catch the RingBuilder error, go fix the file, and expect to be able to retry without making a new CompositeRingBuilder. (Or worse yet, they reach into *this* CompositeRingBuilder, fix the builder, retry, and find that they\u0027ve rebalanced *some* rings twice.)","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                }"},{"line_number":651,"context_line":"                builder.validate()"},{"line_number":652,"context_line":"            except RingBuilderError as err:"},{"line_number":653,"context_line":"                raise RingBuilderError("},{"line_number":654,"context_line":"                    \u0027An error occurred while rebalancing component %s: %s\u0027 %"},{"line_number":655,"context_line":"                    (builder_file, err))"},{"line_number":656,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_01f9347f","line":653,"in_reply_to":"1f1a1f67_29450b71","updated":"2017-07-21 21:14:43.000000000","message":"I think Tim has a point - some of self._builder may been rebalanced, others not so it is safest to discard them all - the ok builders have not yet been saved but their state might otherwise be re-used in a subsequent rebalance.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9729023e6d0fbd3484be285da54c816b7c732f26","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                }"},{"line_number":651,"context_line":"                builder.validate()"},{"line_number":652,"context_line":"            except RingBuilderError as err:"},{"line_number":653,"context_line":"                raise RingBuilderError("},{"line_number":654,"context_line":"                    \u0027An error occurred while rebalancing component %s: %s\u0027 %"},{"line_number":655,"context_line":"                    (builder_file, err))"},{"line_number":656,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_29450b71","line":653,"in_reply_to":"1f1a1f67_9aad8dec","updated":"2017-07-21 07:43:44.000000000","message":"it seems unnecessary, maybe? that is because the already-rebalanced builder is not yet saved. (that will be saved at Line 658 so that even it\u0027s not a new CompositeRingBuilder, this rebalance work spreads all component rings again?","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9729023e6d0fbd3484be285da54c816b7c732f26","unresolved":false,"context_lines":[{"line_number":667,"context_line":"    During rebalance this subclass will consult with any co-builders before"},{"line_number":668,"context_line":"    moving a partition. If a partition has been moved by a co-builder during"},{"line_number":669,"context_line":"    the last `min_part_hours` then it will be treated in the same way as if"},{"line_number":670,"context_line":"    moved by this builder in the last `min_part_hours`."},{"line_number":671,"context_line":""},{"line_number":672,"context_line":"    :param part_power: number of partitions \u003d 2**part_power."},{"line_number":673,"context_line":"    :param replicas: number of replicas for each partition."}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_e90cb343","line":670,"updated":"2017-07-21 07:43:44.000000000","message":"Hmmm....  from Tim\u0027s comment at L522, I\u0027m realizing CooperativeRingBuilder has complexity to mange the cobuilders each other because CooperativeRinBuilder requires cobuilders list for init arg but it needs to include itself. I\u0027m feeling the complexity is from that CooperativeRingBuilder is flatten with cobuilders (i.e. it should refer the list each other so the hierarchy is like as:\n\n(abbreviation: CooperativeRingBuilder -\u003e CB)\n\nCB \u003c-\u003e CB \u003c-\u003e CB .... \u003c-\u003e CB\n\n\nBut perhaps, making a higher tier likely a bucket/list/set could be easy to maintain the components like:\n\n\nbucket\n   |\n----------\n|  |  |  |\nCB CB CB CB\n\nHmm... it seems like just a:\n\nCompositeRingBuilder\n   |\n----------\n|  |  |  |\nCB CB CB CB\n\nAnd then, moving the override change for _can_part_move and _update_last_part_move into CompositeRingBuilder.rebalance would be simpler, perhaps???","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":667,"context_line":"    During rebalance this subclass will consult with any co-builders before"},{"line_number":668,"context_line":"    moving a partition. If a partition has been moved by a co-builder during"},{"line_number":669,"context_line":"    the last `min_part_hours` then it will be treated in the same way as if"},{"line_number":670,"context_line":"    moved by this builder in the last `min_part_hours`."},{"line_number":671,"context_line":""},{"line_number":672,"context_line":"    :param part_power: number of partitions \u003d 2**part_power."},{"line_number":673,"context_line":"    :param replicas: number of replicas for each partition."}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_26f7aef8","line":670,"in_reply_to":"1f1a1f67_e90cb343","updated":"2017-07-21 21:14:43.000000000","message":"Done","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":673,"context_line":"    :param replicas: number of replicas for each partition."},{"line_number":674,"context_line":"    :param min_part_hours: minimum number of hours between partition changes."},{"line_number":675,"context_line":"    :param cobuilders: a list of instances of"},{"line_number":676,"context_line":"        :class:`swift.common.ring.builder.RingBuilder` that will be consulted"},{"line_number":677,"context_line":"        before a part is moved during rebalance; callers should ensure that"},{"line_number":678,"context_line":"        this list includes this instance."},{"line_number":679,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_421dd50d","line":676,"range":{"start_line":676,"start_character":8,"end_line":676,"end_character":54},"updated":"2017-07-20 20:54:12.000000000","message":"I\u0027m pretty sure these all have to be CooperativeRingBuilder instances. Otherwise, the super(CooperativeRingBuilder, b) calls will fail.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":673,"context_line":"    :param replicas: number of replicas for each partition."},{"line_number":674,"context_line":"    :param min_part_hours: minimum number of hours between partition changes."},{"line_number":675,"context_line":"    :param cobuilders: a list of instances of"},{"line_number":676,"context_line":"        :class:`swift.common.ring.builder.RingBuilder` that will be consulted"},{"line_number":677,"context_line":"        before a part is moved during rebalance; callers should ensure that"},{"line_number":678,"context_line":"        this list includes this instance."},{"line_number":679,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_e6d88680","line":676,"range":{"start_line":676,"start_character":8,"end_line":676,"end_character":54},"in_reply_to":"1f1a1f67_421dd50d","updated":"2017-07-21 21:14:43.000000000","message":"Done","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":674,"context_line":"    :param min_part_hours: minimum number of hours between partition changes."},{"line_number":675,"context_line":"    :param cobuilders: a list of instances of"},{"line_number":676,"context_line":"        :class:`swift.common.ring.builder.RingBuilder` that will be consulted"},{"line_number":677,"context_line":"        before a part is moved during rebalance; callers should ensure that"},{"line_number":678,"context_line":"        this list includes this instance."},{"line_number":679,"context_line":"    \"\"\""},{"line_number":680,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":681,"context_line":"        super(CooperativeRingBuilder, self).__init__("}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_d34bf55c","line":678,"range":{"start_line":677,"start_character":49,"end_line":678,"end_character":40},"updated":"2017-07-20 20:54:12.000000000","message":"This seems really clunky... but it looks like Clay already argued you away from adding ourselves to cobuilders...\n\nHmm...\n\nWhat about something like http://paste.openstack.org/show/616082/? Instead of each cobuilder holding a reference to the list of all cobuilders, give \u0027em a reference to the parent CompositeBuilder, and punt to *that* for _can_part_move / _update_last_part_moves.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":674,"context_line":"    :param min_part_hours: minimum number of hours between partition changes."},{"line_number":675,"context_line":"    :param cobuilders: a list of instances of"},{"line_number":676,"context_line":"        :class:`swift.common.ring.builder.RingBuilder` that will be consulted"},{"line_number":677,"context_line":"        before a part is moved during rebalance; callers should ensure that"},{"line_number":678,"context_line":"        this list includes this instance."},{"line_number":679,"context_line":"    \"\"\""},{"line_number":680,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":681,"context_line":"        super(CooperativeRingBuilder, self).__init__("}],"source_content_type":"text/x-python","patch_set":7,"id":"ff346bd7_c6e5823a","line":678,"range":{"start_line":677,"start_character":49,"end_line":678,"end_character":40},"in_reply_to":"1f1a1f67_d34bf55c","updated":"2017-07-21 21:14:43.000000000","message":"Done","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"42c90c904a79ff1fb18956452daddf0c4fe7c6fa","unresolved":false,"context_lines":[{"line_number":680,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":681,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":682,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":683,"context_line":"        self._cobuilders \u003d cobuilders if cobuilders is not None else [self]"},{"line_number":684,"context_line":""},{"line_number":685,"context_line":"    def _can_part_move(self, part):"},{"line_number":686,"context_line":"        return all(super(CooperativeRingBuilder, b)._can_part_move(part)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_e29981d0","line":683,"range":{"start_line":683,"start_character":38,"end_line":683,"end_character":75},"updated":"2017-07-20 20:54:12.000000000","message":"Is this ever tripped in practice, or just in tests? Maybe better to just fix the tests?","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7593fc58e65afc96260a44b175cf5e8928cca44f","unresolved":false,"context_lines":[{"line_number":680,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":681,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":682,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":683,"context_line":"        self._cobuilders \u003d cobuilders if cobuilders is not None else [self]"},{"line_number":684,"context_line":""},{"line_number":685,"context_line":"    def _can_part_move(self, part):"},{"line_number":686,"context_line":"        return all(super(CooperativeRingBuilder, b)._can_part_move(part)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_6f8e6328","line":683,"range":{"start_line":683,"start_character":38,"end_line":683,"end_character":75},"in_reply_to":"1f1a1f67_a57381b1","updated":"2017-07-21 21:14:43.000000000","message":"This ensures that CooperativeRingBuilder will behave exactly the same as a RingBuilder if it not passed cobuilders. I think that is a useful property that could save a nasty suprise if someone instantiates a CooperativeRingBuilder but have no cobuilders, instead of using a RingBuilder - the rebalance will work. The alternative would be to raise an Exception somewhere if cobuilders is None.\n\nupdate: No longer applies in next version.","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9729023e6d0fbd3484be285da54c816b7c732f26","unresolved":false,"context_lines":[{"line_number":680,"context_line":"    def __init__(self, part_power, replicas, min_part_hours, cobuilders):"},{"line_number":681,"context_line":"        super(CooperativeRingBuilder, self).__init__("},{"line_number":682,"context_line":"            part_power, replicas, min_part_hours)"},{"line_number":683,"context_line":"        self._cobuilders \u003d cobuilders if cobuilders is not None else [self]"},{"line_number":684,"context_line":""},{"line_number":685,"context_line":"    def _can_part_move(self, part):"},{"line_number":686,"context_line":"        return all(super(CooperativeRingBuilder, b)._can_part_move(part)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_a57381b1","line":683,"range":{"start_line":683,"start_character":38,"end_line":683,"end_character":75},"in_reply_to":"1f1a1f67_e29981d0","updated":"2017-07-21 07:43:44.000000000","message":"so if someone set cobuilders as None, the reference for _cobuilders is only in CooperativeRIngBuilder instance. it sounds brittle...","commit_id":"593a306e60f5703df3fcfb0f8e98c623da5464ef"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":638,"context_line":"        :raises RingBuilderError: if there is an error while rebalancing any"},{"line_number":639,"context_line":"            component builder."},{"line_number":640,"context_line":"        \"\"\""},{"line_number":641,"context_line":"        self._load_components()"},{"line_number":642,"context_line":"        component_builders \u003d zip(self._builder_files, self._builders)"},{"line_number":643,"context_line":"        # don\u0027t let the same builder go first each time"},{"line_number":644,"context_line":"        shuffle(component_builders)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_1d06cdcf","line":641,"updated":"2017-07-26 10:16:02.000000000","message":"nits) I\u0027m wondering why we don\u0027t need to force the reloading component rings as well as compose. That\u0027s ok, currently we have different behaviors between rebalance and compose.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"27fd9f3ac2a2c877d4501571b779ddaeb350c1ab","unresolved":false,"context_lines":[{"line_number":710,"context_line":"        # override superclass method to delegate to the parent builder"},{"line_number":711,"context_line":"        return self.parent_builder.can_part_move(part)"},{"line_number":712,"context_line":""},{"line_number":713,"context_line":"    def can_part_move(self, part):"},{"line_number":714,"context_line":"        \"\"\""},{"line_number":715,"context_line":"        Check that in the context of this builder alone it is ok to move a"},{"line_number":716,"context_line":"        partition."}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_5c5ba286","line":713,"updated":"2017-07-22 00:40:06.000000000","message":"Now that I think about it, it seems kinda weird that can_part_move wasn\u0027t \"public\" before... whatever.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":720,"context_line":"        \"\"\""},{"line_number":721,"context_line":"        # called by parent_builder - now forward to the superclass"},{"line_number":722,"context_line":"        return (not self._last_part_moves or"},{"line_number":723,"context_line":"                super(CooperativeRingBuilder, self)._can_part_move(part))"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"    def _update_last_part_moves(self):"},{"line_number":726,"context_line":"        # overrides superclass method to delegate to parent builder"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_3d1d11bd","line":723,"updated":"2017-07-26 10:16:02.000000000","message":"OK. The call flow is\n\nCooperativeRingBuilder.rebalance -\u003e CooperativeRingBuilder._can_part_move -\u003e CompositeRing.can_part_move -\u003e CooperativeRingBuiilder.can_part_move -\u003e RingBuilder._can_part_move\n\nThose inline docs helped me, thanks Alistair.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"}],"test/unit/common/ring/test_composite_builder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8953afc9660abc435202bf157dc9626fdbb4c144","unresolved":false,"context_lines":[{"line_number":683,"context_line":"            self.assertIn(\"Invalid builder change at index 0\", line)"},{"line_number":684,"context_line":"            self.assertIn(\"Attribute mismatch for replicas\", line)"},{"line_number":685,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":686,"context_line":""},{"line_number":687,"context_line":"    def test_load_components_insufficient_builders(self):"},{"line_number":688,"context_line":"        def do_test(builder_files):"},{"line_number":689,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_af7a5b85","line":686,"updated":"2017-06-29 14:44:25.000000000","message":"The following new tests almost duplicate many of the existing tests for compose(), because compose() also allows builder files to be loaded asa  side effect. I\u0027d prefer to eliminate that side effect, but that is probably too much for this patch.","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"cf925a6c2047c71dd5349ac2e1a5623f25424bb5","unresolved":false,"context_lines":[{"line_number":683,"context_line":"            self.assertIn(\"Invalid builder change at index 0\", line)"},{"line_number":684,"context_line":"            self.assertIn(\"Attribute mismatch for replicas\", line)"},{"line_number":685,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":686,"context_line":""},{"line_number":687,"context_line":"    def test_load_components_insufficient_builders(self):"},{"line_number":688,"context_line":"        def do_test(builder_files):"},{"line_number":689,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_c5a1c8ab","line":686,"in_reply_to":"3f1d235d_342d5795","updated":"2017-06-30 20:50:46.000000000","message":"I changed the compose behaviour. It\u0027s stupid and will be a pain to live with. Made the tests uniform for both compose and load_components","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9db31e194bf26c46dce62e3241dc3783efc8b5cb","unresolved":false,"context_lines":[{"line_number":683,"context_line":"            self.assertIn(\"Invalid builder change at index 0\", line)"},{"line_number":684,"context_line":"            self.assertIn(\"Attribute mismatch for replicas\", line)"},{"line_number":685,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":686,"context_line":""},{"line_number":687,"context_line":"    def test_load_components_insufficient_builders(self):"},{"line_number":688,"context_line":"        def do_test(builder_files):"},{"line_number":689,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_342d5795","line":686,"in_reply_to":"3f1d235d_af7a5b85","updated":"2017-06-30 14:38:54.000000000","message":"not if this patch further entrenches the undesirable behavior with a bunch of tests!\n\nIME, it\u0027s almost harder to remove tests than it is the behaviors of internal interfaces being tested [1]!\n\n... except I realize now you\u0027re entrenching the *desirable* behavior - and avoiding the difficult work of removing the existing tests we don\u0027t want... hrm... well at least you and I know we want to get rid of them - maybe we can explain it to someone else!?  I\u0027m sure lots of other core maintainers wouldn\u0027t even notice/care, and basically all but one of swift\u0027s operator/deployers will be entirely unaffected by the behavior change on this mostly unexposed interface.  hrm...\n\n1. this observation has been bothering me lately so I might be sensitive to it - but I feel like we had some useful test refactoring that died on the vine because of the risk of \"losing\" a test.  Sigh... I keep thinking it\u0027s a tooling problem - that I need to gear up contextual diffs better - but I think it\u0027s also like some weird kind of FOMO and we can loose site of the good value of a necessary cleanup - or even purge!","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9db31e194bf26c46dce62e3241dc3783efc8b5cb","unresolved":false,"context_lines":[{"line_number":714,"context_line":"        self.add_dev_and_rebalance(builders[1])"},{"line_number":715,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":716,"context_line":"        cb.load_components(other_files)"},{"line_number":717,"context_line":"        self.assertEqual(other_files, cb._builder_files)"},{"line_number":718,"context_line":"        for i, builder in enumerate(cb._builders):"},{"line_number":719,"context_line":"            self.assertEqual(builders[i].id, builder.id)"},{"line_number":720,"context_line":"            self.assertEqual(builders[i].devs, builder.devs)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_54d33339","line":717,"updated":"2017-06-30 14:38:54.000000000","message":"oh i like that.","commit_id":"4903452590c955cf854cda5e2afd21dfc59fad8e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":468,"context_line":"        # modify builders and save in different files"},{"line_number":469,"context_line":"        self.add_dev_and_rebalance(builders[1])"},{"line_number":470,"context_line":"        with self.assertRaises(ValueError):"},{"line_number":471,"context_line":"            cb.compose(builder_files)  # sanity check - originals are unchanged"},{"line_number":472,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":473,"context_line":"        cb.compose(other_files).save(self.output_ring)"},{"line_number":474,"context_line":"        self.check_composite_ring(self.output_ring, builders)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_483d5dc2","side":"PARENT","line":471,"updated":"2017-07-18 11:33:32.000000000","message":"I think the desirable change is just removing \"with\" block, right?\n\nbecause, with this change, the compose method can rebuild the composite ring even if it\u0027s not unchanged.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":468,"context_line":"        # modify builders and save in different files"},{"line_number":469,"context_line":"        self.add_dev_and_rebalance(builders[1])"},{"line_number":470,"context_line":"        with self.assertRaises(ValueError):"},{"line_number":471,"context_line":"            cb.compose(builder_files)  # sanity check - originals are unchanged"},{"line_number":472,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":473,"context_line":"        cb.compose(other_files).save(self.output_ring)"},{"line_number":474,"context_line":"        self.check_composite_ring(self.output_ring, builders)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_e739aa3f","side":"PARENT","line":471,"in_reply_to":"1f1a1f67_2a8099f4","updated":"2017-07-20 07:30:53.000000000","message":"OIC, the compose is meaningless until the builder will be saved. Make sense. OK, your code looks good to me.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":468,"context_line":"        # modify builders and save in different files"},{"line_number":469,"context_line":"        self.add_dev_and_rebalance(builders[1])"},{"line_number":470,"context_line":"        with self.assertRaises(ValueError):"},{"line_number":471,"context_line":"            cb.compose(builder_files)  # sanity check - originals are unchanged"},{"line_number":472,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":473,"context_line":"        cb.compose(other_files).save(self.output_ring)"},{"line_number":474,"context_line":"        self.check_composite_ring(self.output_ring, builders)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_2a8099f4","side":"PARENT","line":471,"in_reply_to":"1f1a1f67_483d5dc2","updated":"2017-07-19 19:44:58.000000000","message":"hmmm, not really because we have done a compose() already in  self._make_composite_builder so there is no point repeating it here - the sanity check became pointless. But I *could* add the require_modified flag and thereby retain the sanity check.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":470,"context_line":"        with self.assertRaises(ValueError):"},{"line_number":471,"context_line":"            cb.compose(builder_files)  # sanity check - originals are unchanged"},{"line_number":472,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":473,"context_line":"        cb.compose(other_files).save(self.output_ring)"},{"line_number":474,"context_line":"        self.check_composite_ring(self.output_ring, builders)"},{"line_number":475,"context_line":"        # check composite builder persists ok"},{"line_number":476,"context_line":"        cb.save(cb_file)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_2aa539a2","side":"PARENT","line":473,"updated":"2017-07-19 19:44:58.000000000","message":"add require_modified flag here as sanity check that the other files are used.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":504,"context_line":"        finally:"},{"line_number":505,"context_line":"            os.chdir(cwd)"},{"line_number":506,"context_line":""},{"line_number":507,"context_line":"    def test_compose_insufficient_builders(self):"},{"line_number":508,"context_line":"        def do_test(builder_files):"},{"line_number":509,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"},{"line_number":510,"context_line":"            with self.assertRaises(ValueError) as cm:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_e88b0965","side":"PARENT","line":507,"updated":"2017-07-18 11:33:32.000000000","message":"Whooa!? Could we remove those tests for compose_ring????","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":504,"context_line":"        finally:"},{"line_number":505,"context_line":"            os.chdir(cwd)"},{"line_number":506,"context_line":""},{"line_number":507,"context_line":"    def test_compose_insufficient_builders(self):"},{"line_number":508,"context_line":"        def do_test(builder_files):"},{"line_number":509,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"},{"line_number":510,"context_line":"            with self.assertRaises(ValueError) as cm:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_8761d614","side":"PARENT","line":507,"in_reply_to":"1f1a1f67_aa1ca977","updated":"2017-07-20 07:30:53.000000000","message":"ack, I didn\u0027t compare TestComposeLoadComponents test suite, yet but it sounds like better reason to move forward.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":504,"context_line":"        finally:"},{"line_number":505,"context_line":"            os.chdir(cwd)"},{"line_number":506,"context_line":""},{"line_number":507,"context_line":"    def test_compose_insufficient_builders(self):"},{"line_number":508,"context_line":"        def do_test(builder_files):"},{"line_number":509,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"},{"line_number":510,"context_line":"            with self.assertRaises(ValueError) as cm:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_aa1ca977","side":"PARENT","line":507,"in_reply_to":"1f1a1f67_e88b0965","updated":"2017-07-19 19:44:58.000000000","message":"They are not removed - they are refactored to TestComposeLoadComponents\n\nWe have the same semantic for supplying builder_files to compose and load_components (it is a *pain* I now wish we had always had two separate method - one to load_components, one to compose, but that is history !).\nI had a choice - repeat exactly same suite of tests for load_components, or refactor to use a common set of tests for each method. Apart from a one time structural diff now, I felt the refactor would be easier to maintain, and shows that the semantics for each method are the same.","commit_id":"a7c8becd4ed41dbf1c88d36b8601d965d6e7cb75"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5a1d68f796e4247325f96289e342675f2afb359d","unresolved":false,"context_lines":[{"line_number":584,"context_line":"                             sorted([b.version for b in rebalance_calls]))"},{"line_number":585,"context_line":"            for b in rebalance_calls:"},{"line_number":586,"context_line":"                self.assertEqual(set(rebalance_calls.keys()),"},{"line_number":587,"context_line":"                                 set(b._cobuilders))"},{"line_number":588,"context_line":""},{"line_number":589,"context_line":"            # check the rebalanced builders were saved"},{"line_number":590,"context_line":"            written_builders \u003d [RingBuilder.load(f) for f in builder_files]"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f1d235d_9684240e","line":587,"updated":"2017-07-01 00:25:10.000000000","message":"the set of rings that had rebalance called on them is exactly equal to the set of rings that each builder knows as cobuilders\n\n\u003d\u003e each builder considers itself a cobuilder\n\n\u003d\u003e this is what we want\n\n... took me a minute ;)\n\nBut this was the test that kept breaking when I screwed this up so I figured I better get my grok on :D","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":602,"context_line":"            self.assertEqual(expected_ids, [r[\u0027builder\u0027].id for r in results])"},{"line_number":603,"context_line":"            self.assertEqual("},{"line_number":604,"context_line":"                [rebalance_calls[r[\u0027builder\u0027]][0] for r in results],"},{"line_number":605,"context_line":"                [r[\u0027result\u0027] for r in results])"},{"line_number":606,"context_line":""},{"line_number":607,"context_line":"        # N.B. the sample builders have zero min_part_hours"},{"line_number":608,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_c24ee96d","line":605,"updated":"2017-07-18 11:33:32.000000000","message":"I\u0027d *really* love to add one more sanity that ONLY ONE REPLICA MOVED for each part across the component rings because it\u0027s a main purpose we want to support the *rebalance* works with *co-builders*, right?\n\nIf you agreed that, I could work on writing the tests tomorrow.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":602,"context_line":"            self.assertEqual(expected_ids, [r[\u0027builder\u0027].id for r in results])"},{"line_number":603,"context_line":"            self.assertEqual("},{"line_number":604,"context_line":"                [rebalance_calls[r[\u0027builder\u0027]][0] for r in results],"},{"line_number":605,"context_line":"                [r[\u0027result\u0027] for r in results])"},{"line_number":606,"context_line":""},{"line_number":607,"context_line":"        # N.B. the sample builders have zero min_part_hours"},{"line_number":608,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_d9f578cc","line":605,"in_reply_to":"1f1a1f67_154dfe3c","updated":"2017-07-20 07:30:53.000000000","message":"OIC. I was missing that. Thanks for pointing out it!.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":602,"context_line":"            self.assertEqual(expected_ids, [r[\u0027builder\u0027].id for r in results])"},{"line_number":603,"context_line":"            self.assertEqual("},{"line_number":604,"context_line":"                [rebalance_calls[r[\u0027builder\u0027]][0] for r in results],"},{"line_number":605,"context_line":"                [r[\u0027result\u0027] for r in results])"},{"line_number":606,"context_line":""},{"line_number":607,"context_line":"        # N.B. the sample builders have zero min_part_hours"},{"line_number":608,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_154dfe3c","line":605,"in_reply_to":"1f1a1f67_c24ee96d","updated":"2017-07-19 19:44:58.000000000","message":"There is an assertion for mutual exclusivity of moved parts here [1] - if you can see a way to more coverage then great.\n\n[1] https://github.com/openstack/swift/blob/f7e5b1f44f6e4392f31e6f15263e16b4538d696c/test/unit/common/ring/test_composite_builder.py#L1016","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"08dd9ba864309592478571b6a3234b6d397f4904","unresolved":false,"context_lines":[{"line_number":941,"context_line":""},{"line_number":942,"context_line":""},{"line_number":943,"context_line":"class TestCooperativeRingBuilder(BaseTestCompositeBuilder):"},{"line_number":944,"context_line":"    def _make_coop_builder(self, region, cobuilders, rebalance\u003dFalse):"},{"line_number":945,"context_line":"        rb \u003d CooperativeRingBuilder(8, 3, 1, cobuilders)"},{"line_number":946,"context_line":"        if cobuilders is not None:"},{"line_number":947,"context_line":"            cobuilders.append(rb)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_cd9cdfc1","line":944,"range":{"start_line":944,"start_character":63,"end_line":944,"end_character":68},"updated":"2017-07-18 11:33:32.000000000","message":"So the proof of pain is here, that shows, we can accept to feed *non-rebalanced ring* into a composite ring. Is that painful????\n\nHowever, test_compose_rings_rebalance_needed didn\u0027t fail with this change so...\n\n(p.s. after looking at composite ring builder change)\n\nThe reason, test_compose_rings_rebalance_needed didn\u0027t fail, is the existing check is still available and no CompositeRingBuilder.compose() call in TestCooperativeRingBuilder with the non-rebalanced CooperativeRingBuilder instances.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"824d842718aa64f6682f82c2fcfedfe61b1bbba7","unresolved":false,"context_lines":[{"line_number":941,"context_line":""},{"line_number":942,"context_line":""},{"line_number":943,"context_line":"class TestCooperativeRingBuilder(BaseTestCompositeBuilder):"},{"line_number":944,"context_line":"    def _make_coop_builder(self, region, cobuilders, rebalance\u003dFalse):"},{"line_number":945,"context_line":"        rb \u003d CooperativeRingBuilder(8, 3, 1, cobuilders)"},{"line_number":946,"context_line":"        if cobuilders is not None:"},{"line_number":947,"context_line":"            cobuilders.append(rb)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_8718b684","line":944,"range":{"start_line":944,"start_character":63,"end_line":944,"end_character":68},"in_reply_to":"1f1a1f67_505824f8","updated":"2017-07-20 07:30:53.000000000","message":"So that means a new staff that non-rebalanced component can be feeded, is coming in this patch. it is designed. Make sense and the test for the situation is better to have. Thanks for adding the test!","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"235d45026d4c90cfe852b5fa1eb17c8f7c6a9f5c","unresolved":false,"context_lines":[{"line_number":941,"context_line":""},{"line_number":942,"context_line":""},{"line_number":943,"context_line":"class TestCooperativeRingBuilder(BaseTestCompositeBuilder):"},{"line_number":944,"context_line":"    def _make_coop_builder(self, region, cobuilders, rebalance\u003dFalse):"},{"line_number":945,"context_line":"        rb \u003d CooperativeRingBuilder(8, 3, 1, cobuilders)"},{"line_number":946,"context_line":"        if cobuilders is not None:"},{"line_number":947,"context_line":"            cobuilders.append(rb)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_505824f8","line":944,"range":{"start_line":944,"start_character":63,"end_line":944,"end_character":68},"in_reply_to":"1f1a1f67_cd9cdfc1","updated":"2017-07-19 19:44:58.000000000","message":"I\u0027m not sure I understand your concern here. \n\n- It is ok for a non-rebalanced component can be passed to a composite ring.\n\n- CompositeRingBuilder.compose() would fail if that component is not first rebalanced.\n\n- It is ok to use CompositeRingBuilder.rebalance() to rebalance that component, and then compose() will succeed.\n\nI will add a test \u0027test_rebalance_with_unrebalanced_builders\u0027 based on your gist.","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5a1d68f796e4247325f96289e342675f2afb359d","unresolved":false,"context_lines":[{"line_number":1035,"context_line":"        # but cobuilders will not prevent a builder rebalancing for first time"},{"line_number":1036,"context_line":"        rb4 \u003d self._make_coop_builder(4, cobuilders, rebalance\u003dFalse)"},{"line_number":1037,"context_line":"        num_moved, _, _ \u003d rb4.rebalance()"},{"line_number":1038,"context_line":"        self.assertEqual(3 * 256, num_moved)"},{"line_number":1039,"context_line":""},{"line_number":1040,"context_line":"    def test_rebalance_cobuilders(self):"},{"line_number":1041,"context_line":"        # verify that co-builder methods are called during one builder\u0027s"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f1d235d_769ce85c","line":1038,"updated":"2017-07-01 00:25:10.000000000","message":"i like this test :)","commit_id":"f7e5b1f44f6e4392f31e6f15263e16b4538d696c"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":504,"context_line":"        finally:"},{"line_number":505,"context_line":"            os.chdir(cwd)"},{"line_number":506,"context_line":""},{"line_number":507,"context_line":"    def test_compose_insufficient_builders(self):"},{"line_number":508,"context_line":"        def do_test(builder_files):"},{"line_number":509,"context_line":"            cb \u003d CompositeRingBuilder(builder_files)"},{"line_number":510,"context_line":"            with self.assertRaises(ValueError) as cm:"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_2e73954f","side":"PARENT","line":507,"updated":"2017-07-26 10:16:02.000000000","message":"This test was moved to test_load_components_insufficient_builders","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":523,"context_line":"        do_test([])"},{"line_number":524,"context_line":"        do_test(builder_files[:1])"},{"line_number":525,"context_line":""},{"line_number":526,"context_line":"    def test_compose_missing_builder_id(self):"},{"line_number":527,"context_line":"        def check_missing_id(cb, builders):"},{"line_number":528,"context_line":"            # not ok to compose with builder_files that have no id assigned"},{"line_number":529,"context_line":"            orig_version \u003d cb.version"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_6e799d2d","side":"PARENT","line":526,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_missing_builder_id","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":551,"context_line":"        cb.compose()  # cb now has component metadata"},{"line_number":552,"context_line":"        check_missing_id(cb, builders)"},{"line_number":553,"context_line":""},{"line_number":554,"context_line":"    def test_compose_duplicate_builder_ids(self):"},{"line_number":555,"context_line":"        builders \u003d self.create_sample_ringbuilders(3)"},{"line_number":556,"context_line":"        builders[2]._id \u003d builders[0]._id"},{"line_number":557,"context_line":"        cb \u003d CompositeRingBuilder(self.save_builders(builders))"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_ceb2c9e6","side":"PARENT","line":554,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_duplicate_builder_ids","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":563,"context_line":"        self.assertFalse(error_lines[1:])"},{"line_number":564,"context_line":"        self.assertEqual(0, cb.version)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"    def test_compose_ring_unchanged_builders(self):"},{"line_number":567,"context_line":"        def do_test(cb, builder_files):"},{"line_number":568,"context_line":"            with self.assertRaises(ValueError) as cm:"},{"line_number":569,"context_line":"                cb.compose(builder_files)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_2ec1552f","side":"PARENT","line":566,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_unchanged_builders","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":584,"context_line":"        builder_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":585,"context_line":"        do_test(cb, builder_files)"},{"line_number":586,"context_line":""},{"line_number":587,"context_line":"    def test_compose_older_builder(self):"},{"line_number":588,"context_line":"        # make first version of composite ring"},{"line_number":589,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":590,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_6e5bdd53","side":"PARENT","line":587,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_older_builder","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":615,"context_line":"        self.assertFalse(error_lines[1:])"},{"line_number":616,"context_line":"        self.assertEqual(2, cb.version)"},{"line_number":617,"context_line":""},{"line_number":618,"context_line":"    def test_compose_different_number_builders(self):"},{"line_number":619,"context_line":"        # not ok to use a different number of component rings"},{"line_number":620,"context_line":"        builders \u003d self.create_sample_ringbuilders(3)"},{"line_number":621,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders[:2])"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_896a7bb0","side":"PARENT","line":618,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_different_number_builders","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        error_lines \u003d do_test(builders)  # too many"},{"line_number":634,"context_line":"        self.assertIn(\"Unexpected extra builder at index 2\", error_lines[0])"},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"    def test_compose_different_builders(self):"},{"line_number":637,"context_line":"        # not ok to change component rings"},{"line_number":638,"context_line":"        builders \u003d self.create_sample_ringbuilders(3)"},{"line_number":639,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders[:2])"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_a95d3fea","side":"PARENT","line":636,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_different_builders","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":647,"context_line":"        self.assertFalse(error_lines[1:])"},{"line_number":648,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"    def test_compose_different_builder_order(self):"},{"line_number":651,"context_line":"        # not ok to change order of component rings"},{"line_number":652,"context_line":"        builders \u003d self.create_sample_ringbuilders(4)"},{"line_number":653,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_09068bcf","side":"PARENT","line":650,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_component_different_builder_order","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":660,"context_line":"            self.assertIn(\"Attribute mismatch for id\", line)"},{"line_number":661,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"    def test_compose_replica_count_changed(self):"},{"line_number":664,"context_line":"        # not ok to change the number of replicas in a ring"},{"line_number":665,"context_line":"        builders \u003d self.create_sample_ringbuilders(3)"},{"line_number":666,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_a92bff2d","side":"PARENT","line":663,"updated":"2017-07-26 10:16:02.000000000","message":"moved to test_load_components_replica_count_changed","commit_id":"3c11f6b8a89fffa9a44e9396f48e1dc98499c81e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":606,"context_line":"            self.assertEqual(expected_ids, [r[\u0027builder\u0027].id for r in results])"},{"line_number":607,"context_line":"            self.assertEqual("},{"line_number":608,"context_line":"                [rebalance_calls[r[\u0027builder\u0027]][0] for r in results],"},{"line_number":609,"context_line":"                [r[\u0027result\u0027] for r in results])"},{"line_number":610,"context_line":""},{"line_number":611,"context_line":"        # N.B. the sample builders have zero min_part_hours"},{"line_number":612,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_89603b03","line":609,"updated":"2017-07-26 10:16:02.000000000","message":"nits: perhaps, it\u0027s worth to have the assertion (or at least it\u0027s a 3 items tuple format) for rebalance result in the dict.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":646,"context_line":"                    cb.rebalance()"},{"line_number":647,"context_line":"        self.assertIn(\u0027An error occurred while rebalancing component %s\u0027 %"},{"line_number":648,"context_line":"                      builder_files[0], str(cm.exception))"},{"line_number":649,"context_line":"        self.assertIsNone(cb._builders)"},{"line_number":650,"context_line":""},{"line_number":651,"context_line":"        with mock.patch(\u0027swift.common.ring.RingBuilder.validate\u0027,"},{"line_number":652,"context_line":"                        side_effect\u003dRingBuilderError(\u0027test\u0027)):"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_9b73e2b1","line":649,"updated":"2017-07-26 10:16:02.000000000","message":"nice.\n\nnits) thinking of cb._builders should be None if any rings got RingBuilderError, perhaps, randomizing the ring raising an error could be robust as the test?","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":967,"context_line":"    def _make_coop_builder(self, region, composite_builder, rebalance\u003dFalse):"},{"line_number":968,"context_line":"        rb \u003d CooperativeRingBuilder(8, 3, 1, composite_builder)"},{"line_number":969,"context_line":"        if composite_builder._builders is None:"},{"line_number":970,"context_line":"            composite_builder._builders \u003d [rb]"},{"line_number":971,"context_line":"        for i in range(3):"},{"line_number":972,"context_line":"            self.add_dev(rb, region\u003dregion)"},{"line_number":973,"context_line":"        if rebalance:"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_1e180073","line":970,"updated":"2017-07-26 10:16:02.000000000","message":"This is because cooperative ring builder is designed to be called only CompositeRingBuilder with *self*.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":973,"context_line":"        if rebalance:"},{"line_number":974,"context_line":"            rb.rebalance()"},{"line_number":975,"context_line":"            self.assertEqual(self._partition_counts(rb),"},{"line_number":976,"context_line":"                             {0: 256, 1: 256, 2: 256})  # sanity check"},{"line_number":977,"context_line":"        return rb"},{"line_number":978,"context_line":""},{"line_number":979,"context_line":"    def _partition_counts(self, builder, key\u003d\u0027id\u0027):"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_fbf0ae69","line":976,"updated":"2017-07-26 10:16:02.000000000","message":"ok, key is device id and the value is the number of partitions assigned.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":976,"context_line":"                             {0: 256, 1: 256, 2: 256})  # sanity check"},{"line_number":977,"context_line":"        return rb"},{"line_number":978,"context_line":""},{"line_number":979,"context_line":"    def _partition_counts(self, builder, key\u003d\u0027id\u0027):"},{"line_number":980,"context_line":"        \"\"\""},{"line_number":981,"context_line":"        Returns a dictionary mapping the given device key to (number of"},{"line_number":982,"context_line":"        partitions assigned to that key)."}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_bb5c667e","line":979,"range":{"start_line":979,"start_character":40,"end_line":979,"end_character":49},"updated":"2017-07-26 10:16:02.000000000","message":"it looks like key is never used in any callers.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":1034,"context_line":"        self.assertEqual(192, num_moved)"},{"line_number":1035,"context_line":"        self.assertEqual(self._partition_counts(rb1),"},{"line_number":1036,"context_line":"                         {0: 192, 1: 192, 2: 192, 3: 192})"},{"line_number":1037,"context_line":"        self.assertEqual(256, num_parts_can_move(rb2))"},{"line_number":1038,"context_line":"        self.assertEqual(64, num_parts_can_move(rb1))"},{"line_number":1039,"context_line":""},{"line_number":1040,"context_line":"        # rebalancing rb2 - rb2 in isolation could potentially move all parts"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_3ec6244a","line":1037,"updated":"2017-07-26 10:16:02.000000000","message":"Is this actually true? Since rb1 changed 192 partition so rb2 is not able to move those partitions until min_part_hour has passed?\n\nOOOh, i see, I got it.\n\nnum_parts_can_move is number of parts can be moved in *super class* so it will be exactly 256.\n\nAnd hopefully, the result of the number of parts actually able to move will be 64.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":1042,"context_line":"        # only having 64 parts that can move"},{"line_number":1043,"context_line":"        self.add_dev(rb2)"},{"line_number":1044,"context_line":"        num_moved, rb2_parts_moved \u003d do_rebalance(rb2)"},{"line_number":1045,"context_line":"        self.assertEqual(64, num_moved)"},{"line_number":1046,"context_line":"        counts \u003d self._partition_counts(rb2)"},{"line_number":1047,"context_line":"        self.assertEqual(counts[3], 64)"},{"line_number":1048,"context_line":"        self.assertEqual([234, 235, 235], sorted(counts.values()[:3]))"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_7edb8cec","line":1045,"updated":"2017-07-26 10:16:02.000000000","message":"OK, that\u0027s what I thought in the previous comment.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":1112,"context_line":"                rb1.rebalance()"},{"line_number":1113,"context_line":"        self.assertEqual([rb1], update_calls)"},{"line_number":1114,"context_line":"        self.assertEqual([rb1], can_part_move_calls.keys())"},{"line_number":1115,"context_line":"        self.assertEqual(512, len(can_part_move_calls[rb1]))"},{"line_number":1116,"context_line":""},{"line_number":1117,"context_line":"        # two component builders with same parent builder"},{"line_number":1118,"context_line":"        cb \u003d CompositeRingBuilder()"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_311d3566","line":1115,"updated":"2017-07-26 10:16:02.000000000","message":"Ok, i think the parent _can_part_move called twice in _gather_parts_for_dispersion for initial rebalance so the result looks correct.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"4eae17185828ac724a69679d27a000e7e2438060","unresolved":false,"context_lines":[{"line_number":1152,"context_line":""},{"line_number":1153,"context_line":"        # check can be loaded as superclass"},{"line_number":1154,"context_line":"        loaded_rb \u003d RingBuilder.load(builder_file)"},{"line_number":1155,"context_line":"        self.assertEqual(coop_rb.to_dict(), loaded_rb.to_dict())"},{"line_number":1156,"context_line":""},{"line_number":1157,"context_line":"        # check can load a saved superclass"},{"line_number":1158,"context_line":"        rb \u003d RingBuilder(6, 3, 0)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_f1768da4","line":1155,"updated":"2017-07-26 10:16:02.000000000","message":"Oh, it\u0027s interesting.","commit_id":"71ec83f414e81dd6d44ea85da14a7d64c57b5a2b"}]}