)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"In the future work, we will have new commands around swift-ring-builder"},{"line_number":26,"context_line":"to maintain the relation of builder files in the composite ring and"},{"line_number":27,"context_line":"then, that calls the `composite` function with the builder files."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Co-Authored-By: Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"1a1ced50_f72356a5","line":27,"updated":"2017-03-20 11:02:13.000000000","message":"we should not promise future features in a commit message, esepcially when thay are uncertain.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"}],"doc/source/overview_ring.rst":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4d5b7616272d90c3c2947bcbe5e02bfa2411f1b0","unresolved":false,"context_lines":[{"line_number":353,"context_line":"---------------"},{"line_number":354,"context_line":"Composite Rings"},{"line_number":355,"context_line":"---------------"},{"line_number":356,"context_line":".. automodule:: swift.common.ring.composite_builder"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"-------"},{"line_number":359,"context_line":"History"}],"source_content_type":"text/x-rst","patch_set":21,"id":"1f013ff3_e6a16bab","line":356,"updated":"2017-05-16 22:02:40.000000000","message":"Should this (an eventually user-facing feature) be moved above Ring Builder Analyzer (which calls out that it probably isn\u0027t useful unless you\u0027re a developer)?","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"}],"swift/cli/ringbuilder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":471,"context_line":"        except AttributeError:"},{"line_number":472,"context_line":"            builder_id \u003d \"(not assigned)\""},{"line_number":473,"context_line":"        print(\u0027%s, build version %d, id %s\u0027 %"},{"line_number":474,"context_line":"              (builder_file, builder.version, builder_id))"},{"line_number":475,"context_line":"        regions \u003d 0"},{"line_number":476,"context_line":"        zones \u003d 0"},{"line_number":477,"context_line":"        balance \u003d 0"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_a5146ba1","line":474,"updated":"2017-04-26 03:37:58.000000000","message":"looks good to me!\n\nhttps://gist.github.com/clayg/1926c872d033f2e4fd9808078d1d6be3","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"}],"swift/cli/ringzipper.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"358cc6767f5669d23a72c4be45a235d93f0b84c6","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    :param rings: a list of `swift.common.ring.Ring` instances"},{"line_number":40,"context_line":"    \"\"\""},{"line_number":41,"context_line":"    # all rings should be consistent for each MUST_MATCH_ATTRS"},{"line_number":42,"context_line":"    for attr in MUST_MATCH_ATTRS:"},{"line_number":43,"context_line":"        attr_dict \u003d defaultdict(list)"},{"line_number":44,"context_line":"        for ring in rings:"},{"line_number":45,"context_line":"            value \u003d getattr(ring, attr, None)"},{"line_number":46,"context_line":"            attr_dict[value].append(ring)"},{"line_number":47,"context_line":"        if len(attr_dict) \u003e 1:"},{"line_number":48,"context_line":"            # TODO: clean up the message"},{"line_number":49,"context_line":"            raise ValueError("},{"line_number":50,"context_line":"                \u0027Different %s found %s\u0027 % (attr, attr_dict))"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    # check regions"},{"line_number":53,"context_line":"    regions_info \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba2be162_3861c005","line":50,"range":{"start_line":42,"start_character":0,"end_line":50,"end_character":60},"updated":"2017-03-06 14:10:36.000000000","message":"no tests for them, right now.","commit_id":"b1f032f10e624513a0373c09d747dc04983a3682"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"dafbef53ceaad09f6ade1753f485790f08f67a2e","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    :param rings: a list of `swift.common.ring.Ring` instances"},{"line_number":40,"context_line":"    \"\"\""},{"line_number":41,"context_line":"    # all rings should be consistent for each MUST_MATCH_ATTRS"},{"line_number":42,"context_line":"    for attr in MUST_MATCH_ATTRS:"},{"line_number":43,"context_line":"        attr_dict \u003d defaultdict(list)"},{"line_number":44,"context_line":"        for ring in rings:"},{"line_number":45,"context_line":"            value \u003d getattr(ring, attr, None)"},{"line_number":46,"context_line":"            attr_dict[value].append(ring)"},{"line_number":47,"context_line":"        if len(attr_dict) \u003e 1:"},{"line_number":48,"context_line":"            # TODO: clean up the message"},{"line_number":49,"context_line":"            raise ValueError("},{"line_number":50,"context_line":"                \u0027Different %s found %s\u0027 % (attr, attr_dict))"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    # check regions"},{"line_number":53,"context_line":"    regions_info \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_1f5849fa","line":50,"range":{"start_line":42,"start_character":0,"end_line":50,"end_character":60},"in_reply_to":"ba2be162_3861c005","updated":"2017-03-07 08:19:50.000000000","message":"Done","commit_id":"b1f032f10e624513a0373c09d747dc04983a3682"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"06a6c03313068b54babeda0218f10b1e29c44b3e","unresolved":false,"context_lines":[{"line_number":109,"context_line":"    for i, ring in enumerate(rings):"},{"line_number":110,"context_line":"        regions_info[i] \u003d set([dev[\u0027region\u0027] for dev in ring.devs])"},{"line_number":111,"context_line":"    for first, second in combinations(regions_info.items(), 2):"},{"line_number":112,"context_line":"        inter \u003d first[1].intersection(second[1])"},{"line_number":113,"context_line":"        if inter:"},{"line_number":114,"context_line":"            # TODO: it may be useful to ragard the same region in the"},{"line_number":115,"context_line":"            #       different rings as different region. However, on that"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a30ddce_f81b5d7e","line":112,"updated":"2017-03-08 03:32:36.000000000","message":"FWIW, https://docs.python.org/3/library/stdtypes.html#set.intersection isn\u0027t quite right because that only the intersection of *all* the sets - it won\u0027t spot any intersection between *any* of the sets","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"82bd3617a1de800164f1fb517f2f865cc3e02d9b","unresolved":false,"context_lines":[{"line_number":109,"context_line":"    for i, ring in enumerate(rings):"},{"line_number":110,"context_line":"        regions_info[i] \u003d set([dev[\u0027region\u0027] for dev in ring.devs])"},{"line_number":111,"context_line":"    for first, second in combinations(regions_info.items(), 2):"},{"line_number":112,"context_line":"        inter \u003d first[1].intersection(second[1])"},{"line_number":113,"context_line":"        if inter:"},{"line_number":114,"context_line":"            # TODO: it may be useful to ragard the same region in the"},{"line_number":115,"context_line":"            #       different rings as different region. However, on that"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a1ff146_09cb861f","line":112,"in_reply_to":"3a1ff146_8825d0c3","updated":"2017-03-15 23:03:28.000000000","message":"I\u0027m so, sorry!  I was actually trying to re-affirm your code is necessary and sufficient!  My comment was for other reviewers who might think there\u0027s an easier way to do this that what you have written here - and I do not think there is!  This code is very good!","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c7cad438b3e59b41b138eef700c5207dc5faffa8","unresolved":false,"context_lines":[{"line_number":109,"context_line":"    for i, ring in enumerate(rings):"},{"line_number":110,"context_line":"        regions_info[i] \u003d set([dev[\u0027region\u0027] for dev in ring.devs])"},{"line_number":111,"context_line":"    for first, second in combinations(regions_info.items(), 2):"},{"line_number":112,"context_line":"        inter \u003d first[1].intersection(second[1])"},{"line_number":113,"context_line":"        if inter:"},{"line_number":114,"context_line":"            # TODO: it may be useful to ragard the same region in the"},{"line_number":115,"context_line":"            #       different rings as different region. However, on that"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a1ff146_8825d0c3","line":112,"in_reply_to":"9a30ddce_f81b5d7e","updated":"2017-03-15 11:21:52.000000000","message":"can i have the example of worse case? probably I\u0027m missing something on the case.\n\nThis for-loop is asserting all combinations (picking  up any 2 ring files and comparing the sets) for all rings so that like as:\n\nexample-0:\n ring-0: set([region-0])\n ring-1: set([region-1])\n -\u003e safe you know\n\nexample-1:\n ring-0: set([region-0, region-1])\n ring-1: set([region-2, region-3])\n -\u003e this is still safe\n\nexample-2:\n ring-0: set([region-0, region-1])\n ring-1: set([region-2, region-3])\n ring-2: set([region-0, region-4])  # region-0 is covered by ring-0\n -\u003e this case is able to fail \n\nso i think, it\u0027s not all the sets but any of the sets...","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"06a6c03313068b54babeda0218f10b1e29c44b3e","unresolved":false,"context_lines":[{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    # parhaps, ring_files should be sorted by something like... name?"},{"line_number":166,"context_line":"    # the order swapness could be painful operation for us?"},{"line_number":167,"context_line":"    rings \u003d [Ring(ring_file) for ring_file in ring_files]"},{"line_number":168,"context_line":"    try:"},{"line_number":169,"context_line":"        pre_validate_all_rings(rings)"},{"line_number":170,"context_line":"    except ValueError as e:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a30ddce_0399a61f","line":167,"updated":"2017-03-08 03:32:36.000000000","message":"I think here to about line ~L192 should be extracted to function\n\ntake a list of Ring\u0027s, return a zipped RingData - raise ValueError for validation\n\nI also think such a function and it\u0027s supporting validation functions should be swift.common.ring.zipper\n\nleaving *this* module only for pulling arguments from the command line\n\nIt\u0027s not only possible, but likely in my case, that the main way I consume upstream support for zipped rings will be through programatic access via python.\n\nIdeally in my case I\u0027d like to replace a call site like:\n\n    for storage_policy in all_cluster_storage_policies:\n        builder \u003d get_builder_for_storage_policy(storage_policy)\n        # fix builder devices and weights etc\n        builder.rebalance()\n        builder.get_ring().save(get_ring_name_for_storage_policy(storage_policy))\n\nWith something like:\n\n    for storage_policy in all_cluster_storage_policies:\n        builders \u003d get_builders_for_storage_policy(storage_policy)\n        for builder in builders:\n            # fix builder devices and weights etc\n            builder.rebalance()\n        if len(builders) \u003d\u003d 1:\n            builder.get_ring().save(get_ring_name_for_storage_policy(storage_policy))\n        else:\n            MAGIC([b.get_ring() for b in builders], get_ring_name_for_storage_policy(storage_policy))","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"82bd3617a1de800164f1fb517f2f865cc3e02d9b","unresolved":false,"context_lines":[{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    # parhaps, ring_files should be sorted by something like... name?"},{"line_number":166,"context_line":"    # the order swapness could be painful operation for us?"},{"line_number":167,"context_line":"    rings \u003d [Ring(ring_file) for ring_file in ring_files]"},{"line_number":168,"context_line":"    try:"},{"line_number":169,"context_line":"        pre_validate_all_rings(rings)"},{"line_number":170,"context_line":"    except ValueError as e:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a1ff146_8985b6ae","line":167,"in_reply_to":"3a1ff146_23823d53","updated":"2017-03-15 23:03:28.000000000","message":"Sorry, I don\u0027t mean to be so transparently self-interested :\\\n\nMy hope is that being honest about my requirements is acctually *helpful* in that my requirements are not so terribly unrepresentative of a larger ecosystem of consumers.\n\nWe do have some reports that a significant community of deployers/operators running at scale are doing some amount of programatic interfacing with the RingBuilder class directly through python code.","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c7cad438b3e59b41b138eef700c5207dc5faffa8","unresolved":false,"context_lines":[{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    # parhaps, ring_files should be sorted by something like... name?"},{"line_number":166,"context_line":"    # the order swapness could be painful operation for us?"},{"line_number":167,"context_line":"    rings \u003d [Ring(ring_file) for ring_file in ring_files]"},{"line_number":168,"context_line":"    try:"},{"line_number":169,"context_line":"        pre_validate_all_rings(rings)"},{"line_number":170,"context_line":"    except ValueError as e:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a1ff146_23823d53","line":167,"in_reply_to":"9a30ddce_0399a61f","updated":"2017-03-15 11:21:52.000000000","message":"Er ok. This is what you want and it\u0027s why you suggest to make the zip functionality into ringbuilder. I incline to make the zipper into the builder so probably I can address this comments in the future work to progress this patch. Thanks.","commit_id":"1a3c4559a1b45d02ce4e7969bcc12c347e33dc0d"}],"swift/common/ring/builder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.builder_file \u003d builder_file"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_1531f30b","line":96,"updated":"2017-03-17 18:43:42.000000000","message":"this seems scarey i.e. ultimately embedding the file path into the ring metadata. As discussed on etherpad, that is very brittle.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.builder_file \u003d builder_file"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_17147242","line":96,"in_reply_to":"1a1ced50_1531f30b","updated":"2017-03-20 11:02:13.000000000","message":"Thinking more about this: an different idea to using file names and md5 hashes would be to assign each ring (builder or .gz?) a UUID.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"ecba392aca5f9b5221e05a8cf13f0c89e6a9560a","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.builder_file \u003d builder_file"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_eecceabc","line":96,"in_reply_to":"1a1ced50_17147242","updated":"2017-03-22 16:39:07.000000000","message":"We were discussing \"epochs\" in the past, maybe this is a good time to bring this up again?","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.builder_file \u003d builder_file"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_f1ee6a20","line":96,"in_reply_to":"1a1ced50_eecceabc","updated":"2017-03-23 11:01:43.000000000","message":"\u003eAlistair\n\nGood call, actually I was not sure if we *want* to know the filename here. if we need only uuid like md5 hash, it will be easy more. Temporary, I\u0027ll drop this change in the future patch.\n\n\u003eChristian\n\n\"epochs\" means timestamp, right? If you could share what info we need, for the ring maintainance of the ring builder versioning from your operator experience, it will be worthful to improve the ring metadata discussing around here!","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":376,"context_line":"            self.devs.append(None)"},{"line_number":377,"context_line":"        dev[\u0027weight\u0027] \u003d float(dev[\u0027weight\u0027])"},{"line_number":378,"context_line":"        dev[\u0027parts\u0027] \u003d 0"},{"line_number":379,"context_line":"        dev.setdefault(\u0027meta\u0027, \u0027\u0027)"},{"line_number":380,"context_line":"        self.devs[dev[\u0027id\u0027]] \u003d dev"},{"line_number":381,"context_line":"        self.devs_changed \u003d True"},{"line_number":382,"context_line":"        self.version +\u003d 1"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_2426c7ba","line":379,"updated":"2017-03-17 18:43:42.000000000","message":"doesn\u0027t seem to be used","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"ecba392aca5f9b5221e05a8cf13f0c89e6a9560a","unresolved":false,"context_lines":[{"line_number":376,"context_line":"            self.devs.append(None)"},{"line_number":377,"context_line":"        dev[\u0027weight\u0027] \u003d float(dev[\u0027weight\u0027])"},{"line_number":378,"context_line":"        dev[\u0027parts\u0027] \u003d 0"},{"line_number":379,"context_line":"        dev.setdefault(\u0027meta\u0027, \u0027\u0027)"},{"line_number":380,"context_line":"        self.devs[dev[\u0027id\u0027]] \u003d dev"},{"line_number":381,"context_line":"        self.devs_changed \u003d True"},{"line_number":382,"context_line":"        self.version +\u003d 1"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_ceb3ae08","line":379,"in_reply_to":"1a1ced50_2426c7ba","updated":"2017-03-22 16:39:07.000000000","message":"I think this is required if the device is not added by the CLI. The CLI automatically sets this to \u0027\u0027 if not set, so probably required here if used outside the CLI","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":376,"context_line":"            self.devs.append(None)"},{"line_number":377,"context_line":"        dev[\u0027weight\u0027] \u003d float(dev[\u0027weight\u0027])"},{"line_number":378,"context_line":"        dev[\u0027parts\u0027] \u003d 0"},{"line_number":379,"context_line":"        dev.setdefault(\u0027meta\u0027, \u0027\u0027)"},{"line_number":380,"context_line":"        self.devs[dev[\u0027id\u0027]] \u003d dev"},{"line_number":381,"context_line":"        self.devs_changed \u003d True"},{"line_number":382,"context_line":"        self.version +\u003d 1"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_f27bcdf2","line":379,"in_reply_to":"1a1ced50_ceb3ae08","updated":"2017-03-23 11:01:43.000000000","message":"Er - perhaps, this should be in a separate patch though. And thanks Christian, I didn\u0027t notice the CLI fills the \"meta\" key.\n\nThe reason I\u0027d prefer to this is \"builder.to_dict()\" will be inconsistent before saving/after loading if no meta key specified to add_dev method directly (exactly it\u0027s not the CLI case). Without this change, the assertion like:\n\nhttps://gist.github.com/bloodeagle40234/768cd93d541635ec3e0394fe72edc20b\n\nwill go to fail because non-loaded ring builder, i.e. created as an instance directly like rb \u003d RingBuilder, won\u0027t report the \"meta\" key in the \"to_dict()\" result but the \"meta\" will be set as \"\" at the loading so the loaded ring builder, i.e. rb \u003d RingBuilder.load(\u0027builderfile.builder\u0027) will always contain the \"meta\" key. I found the difference when I was writing the testing code (around hashing the to_dict result) and then, to avoid the inconsistency, I added the setdefault here.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04ce25c11ad1e4289b6fc1ce7fab4b8788f24a97","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1671,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1672,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"},{"line_number":1673,"context_line":"        builder.source \u003d builder_file"},{"line_number":1674,"context_line":"        return builder"},{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"    def save(self, builder_file):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_7e2366be","line":1673,"updated":"2017-03-28 08:16:15.000000000","message":"Oops!? builder_file is living?","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"58993016df2bb34633096212136a9cc5eec3930c","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1671,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1672,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"},{"line_number":1673,"context_line":"        builder.source \u003d builder_file"},{"line_number":1674,"context_line":"        return builder"},{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"    def save(self, builder_file):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_47a20bff","line":1673,"in_reply_to":"ffe62b97_7e2366be","updated":"2017-03-28 09:17:12.000000000","message":"Done","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    \"\"\""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def __init__("},{"line_number":77,"context_line":"            self, part_power, replicas, min_part_hours, source\u003dNone):"},{"line_number":78,"context_line":"        if part_power \u003e 32:"},{"line_number":79,"context_line":"            raise ValueError(\"part_power must be at most 32 (was %d)\""},{"line_number":80,"context_line":"                             % (part_power,))"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_268bb21f","line":77,"updated":"2017-04-04 03:30:00.000000000","message":"As best I can tell source is only used for tests - sort of the same way we parameterize logger except that... with logger we\u0027ll make one if it\u0027s not passed in.  The proper way to set source is via load.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.source \u003d source"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_7ee70c42","line":96,"updated":"2017-04-04 03:30:00.000000000","message":"This makes me a little nervous.  There\u0027s a nuanced change that changes the nature of this object from a just a \"Doer\" to a representation of an on-disk state.  Maybe the class methods were already coupled enough to a filesystem path that it doesn\u0027t matter...","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        self.devs_changed \u003d False"},{"line_number":94,"context_line":"        self.version \u003d 0"},{"line_number":95,"context_line":"        self.overload \u003d 0.0"},{"line_number":96,"context_line":"        self.source \u003d source"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        # _replica2part2dev maps from replica number to partition number to"},{"line_number":99,"context_line":"        # device id. So, for a three replica, 2**23 ring, it\u0027s an array of"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_0229b188","line":96,"in_reply_to":"dfeb2761_7ee70c42","updated":"2017-04-04 11:20:04.000000000","message":"by \u0027on-disk state\u0027 you mean the path to builder file? yeah, I see your point that until now the builder object was decoupled from any location in the filesystem from which it was loaded or to which it may be written. Ideally I still would prefer source to be a UUID, *if* we can agree on how to make that work.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1671,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1672,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"},{"line_number":1673,"context_line":"        builder.source \u003d builder_file"},{"line_number":1674,"context_line":"        return builder"},{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"    def save(self, builder_file):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_4673564e","line":1673,"updated":"2017-04-04 03:30:00.000000000","message":"so this attribute is just the path that was used to load the builder *this time* - doesn\u0027t really seem to be state of the Builder but rather the Loader which has been stateless class method thus far.  I don\u0027t *love* it!","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1671,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1672,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"},{"line_number":1673,"context_line":"        builder.source \u003d builder_file"},{"line_number":1674,"context_line":"        return builder"},{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"    def save(self, builder_file):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_6f72a6e5","line":1673,"in_reply_to":"dfeb2761_4673564e","updated":"2017-04-04 11:20:04.000000000","message":"this doesn\u0027t change the on-disk state of the builder. But w/e, I don\u0027t want to defend this too strongly because I\u0027m actually frustrated that we don\u0027t have any perfect way to identify a builder from one instance to another.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            # that case the caller must save the builder in order that a newly"},{"line_number":139,"context_line":"            # assigned id is persisted."},{"line_number":140,"context_line":"            raise AttributeError("},{"line_number":141,"context_line":"                \u0027id attribute has not been initialised by calling save()\u0027)"},{"line_number":142,"context_line":"        return self._id"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_6ad782c1","line":141,"updated":"2017-04-24 21:37:25.000000000","message":"I\u0027m a little worried about the tech debt of always needing to worry about AttributeErrors when using builder.id ... but at least we\u0027re being explicit about it, I suppose. Probably better than just returning None.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d6effeb84c52a685fa72bb4f36b9250b41b16334","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            # that case the caller must save the builder in order that a newly"},{"line_number":139,"context_line":"            # assigned id is persisted."},{"line_number":140,"context_line":"            raise AttributeError("},{"line_number":141,"context_line":"                \u0027id attribute has not been initialised by calling save()\u0027)"},{"line_number":142,"context_line":"        return self._id"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_8534cffc","line":141,"in_reply_to":"5ff73747_6ad782c1","updated":"2017-04-26 02:32:51.000000000","message":"IMHO it\u0027s the opposite of tech debt - instead of trying to paper over it for convenience we made an explicit design choice to force the builder to be saved to get an id - opinionated and enforceable, easy to reason about","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        return self._id"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @property"},{"line_number":145,"context_line":"    def part_shift(self):"},{"line_number":146,"context_line":"        return 32 - self.part_power"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    def _set_part_moved(self, part):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_6dab1cd6","line":145,"updated":"2017-04-24 21:37:25.000000000","message":"Should this be an ordinary attribute, like was done for self.parts? Alternatively, should self.parts be a @property, since it\u0027s *also* derived from self.part_power?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        return self._id"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @property"},{"line_number":145,"context_line":"    def part_shift(self):"},{"line_number":146,"context_line":"        return 32 - self.part_power"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    def _set_part_moved(self, part):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_290e1b8d","line":145,"in_reply_to":"5ff73747_6dab1cd6","updated":"2017-04-25 08:45:31.000000000","message":"Hmm... it would be good call. The reason I made this @property here is the part_shift needed to create a new RingData instance for the composite ring and I\u0027d like to make it as *read only* rather than normal attribute.\n\nSo if we improve around here, I\u0027d vote to change self.parts as a property.\n\nHowever, I\u0027m a bit worried about the change on self.parts could be painful for Christian\u0027s patch 337297, maybe? I\u0027m not sure because I didn\u0027t look at deeply. Probably, you know in detail rather than me, right?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        return {\u0027part_power\u0027: self.part_power,"},{"line_number":264,"context_line":"                \u0027replicas\u0027: self.replicas,"},{"line_number":265,"context_line":"                \u0027min_part_hours\u0027: self.min_part_hours,"},{"line_number":266,"context_line":"                \u0027parts\u0027: self.parts,"},{"line_number":267,"context_line":"                \u0027devs\u0027: self.devs,"},{"line_number":268,"context_line":"                \u0027devs_changed\u0027: self.devs_changed,"},{"line_number":269,"context_line":"                \u0027version\u0027: self.version,"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_ad82444e","line":266,"updated":"2017-04-24 21:37:25.000000000","message":"Off topic: Why do we include a derived value here?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d6effeb84c52a685fa72bb4f36b9250b41b16334","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        return {\u0027part_power\u0027: self.part_power,"},{"line_number":264,"context_line":"                \u0027replicas\u0027: self.replicas,"},{"line_number":265,"context_line":"                \u0027min_part_hours\u0027: self.min_part_hours,"},{"line_number":266,"context_line":"                \u0027parts\u0027: self.parts,"},{"line_number":267,"context_line":"                \u0027devs\u0027: self.devs,"},{"line_number":268,"context_line":"                \u0027devs_changed\u0027: self.devs_changed,"},{"line_number":269,"context_line":"                \u0027version\u0027: self.version,"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_d6f04fcc","line":266,"in_reply_to":"5ff73747_ad82444e","updated":"2017-04-26 02:32:51.000000000","message":"Ring serialization used to just be straight up pickle, so every attribute got serialized/persisted - when it got moved over to dicts and stuff [1] probably just didn\u0027t think to recalculate it - or apply some consistent strategy for doing @property which would probably be a welcome cleanup\n\n1. https://github.com/openstack/swift/commit/fc6391ea","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":1683,"context_line":"            # NOTE(akscram): An old ring builder file don\u0027t contain"},{"line_number":1684,"context_line":"            #                replication parameters."},{"line_number":1685,"context_line":"            if dev:"},{"line_number":1686,"context_line":"                if \u0027ip\u0027 in dev:"},{"line_number":1687,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1688,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1689,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_8a888ec6","line":1686,"updated":"2017-04-24 21:37:25.000000000","message":"Well, at least there\u0027s plenty of precedent for needing to worry about builders with weird/old/bad data in them.\n\nOff topic: How would you even *use* a device that doesn\u0027t have an ip?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d6effeb84c52a685fa72bb4f36b9250b41b16334","unresolved":false,"context_lines":[{"line_number":1683,"context_line":"            # NOTE(akscram): An old ring builder file don\u0027t contain"},{"line_number":1684,"context_line":"            #                replication parameters."},{"line_number":1685,"context_line":"            if dev:"},{"line_number":1686,"context_line":"                if \u0027ip\u0027 in dev:"},{"line_number":1687,"context_line":"                    dev.setdefault(\u0027replication_ip\u0027, dev[\u0027ip\u0027])"},{"line_number":1688,"context_line":"                if \u0027port\u0027 in dev:"},{"line_number":1689,"context_line":"                    dev.setdefault(\u0027replication_port\u0027, dev[\u0027port\u0027])"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_76bf3bd0","line":1686,"in_reply_to":"5ff73747_8a888ec6","updated":"2017-04-26 02:32:51.000000000","message":"I had assumed it was because of some stupid test, but I think it was just a overly defensive/paranioa when replication network got added\n\nhttps://github.com/openstack/swift/commit/ea785817\n\nwe could *try* to clean it up i \u0027spose - https://review.openstack.org/459914","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":1695,"context_line":"        :param builder_file: path to builder file to save"},{"line_number":1696,"context_line":"        \"\"\""},{"line_number":1697,"context_line":"        # We want to be sure the builder id\u0027s are persistent, so this is the"},{"line_number":1698,"context_line":"        # only place where the id is assigned. Newly created instances of this"},{"line_number":1699,"context_line":"        # class, or instances loaded from legacy builder files that have no"},{"line_number":1700,"context_line":"        # persisted id, must be saved in order for an id to be assigned."},{"line_number":1701,"context_line":"        id_persisted \u003d True"},{"line_number":1702,"context_line":"        if self._id is None:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_cd56b007","line":1699,"range":{"start_line":1698,"start_character":47,"end_line":1699,"end_character":15},"updated":"2017-04-24 21:37:25.000000000","message":"This part seems a little odd -- why can\u0027t we go ahead and assign an id for brand new instances?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":1695,"context_line":"        :param builder_file: path to builder file to save"},{"line_number":1696,"context_line":"        \"\"\""},{"line_number":1697,"context_line":"        # We want to be sure the builder id\u0027s are persistent, so this is the"},{"line_number":1698,"context_line":"        # only place where the id is assigned. Newly created instances of this"},{"line_number":1699,"context_line":"        # class, or instances loaded from legacy builder files that have no"},{"line_number":1700,"context_line":"        # persisted id, must be saved in order for an id to be assigned."},{"line_number":1701,"context_line":"        id_persisted \u003d True"},{"line_number":1702,"context_line":"        if self._id is None:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_98633add","line":1699,"range":{"start_line":1698,"start_character":47,"end_line":1699,"end_character":15},"in_reply_to":"5ff73747_cd56b007","updated":"2017-04-25 08:45:31.000000000","message":"Please see the discussion on etherpad[1](around Proposal section from Line 28) for clarify the meaning. In my understand, the id is unique enough with the persistent (in disk) ring builder file and it should be compatible legacy rings.\n\n1. when we assign an id only if it\u0027s a brand new instance, the legacy ring could not get an id.\n\n2. regarding to the A situation (and to solve the issue), if we try to create a brand new instance always when loading the ring, some different ids can be assigned from a legacy ring for each instantiation.\n\n\nConsidering about the item 2, it could be a pain point for the composite ring because composite ring need the builder *instances* to compose and trust the id for the compose order for the builders. However, with the item 2 situation, we could loose the id if something wrong happened *BEFORE* the instance was saved.\n\nAnd then, we won\u0027t be able to assert the lost id for the composite ring management. I think that is the reason why we need the persistent id and it will be assigned only when it\u0027s being persistent.\n\nDoes it make sense?\n\n1: https://etherpad.openstack.org/p/composite_rings","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        # only place where the id is assigned. Newly created instances of this"},{"line_number":1699,"context_line":"        # class, or instances loaded from legacy builder files that have no"},{"line_number":1700,"context_line":"        # persisted id, must be saved in order for an id to be assigned."},{"line_number":1701,"context_line":"        id_persisted \u003d True"},{"line_number":1702,"context_line":"        if self._id is None:"},{"line_number":1703,"context_line":"            id_persisted \u003d False"},{"line_number":1704,"context_line":"            self._id \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_ed03ac0c","line":1701,"updated":"2017-04-24 21:37:25.000000000","message":"I came at this from the perspective of what we\u0027re doing *right now*, rather than what the state was *when we loaded the ring*. Maybe it\u0027s just me, but I think it might be easier to think about if we inverted the logic a bit? Something like\n\n id_autogenerated \u003d False\n if self._id is None:\n     id_autogenerated \u003d True\n     self._id \u003d uuid.uuid4().hex\n try:\n     with open(builder_file, \u0027wb\u0027) as f:\n         pickle.dump(self.to_dict(), f, protocol\u003d2)\n except Exception:\n     if id_autogenerated:\n         self._id \u003d None\n     raise","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d6effeb84c52a685fa72bb4f36b9250b41b16334","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        # only place where the id is assigned. Newly created instances of this"},{"line_number":1699,"context_line":"        # class, or instances loaded from legacy builder files that have no"},{"line_number":1700,"context_line":"        # persisted id, must be saved in order for an id to be assigned."},{"line_number":1701,"context_line":"        id_persisted \u003d True"},{"line_number":1702,"context_line":"        if self._id is None:"},{"line_number":1703,"context_line":"            id_persisted \u003d False"},{"line_number":1704,"context_line":"            self._id \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_c5541724","line":1701,"in_reply_to":"5ff73747_12f9354c","updated":"2017-04-26 02:32:51.000000000","message":"+1","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        # only place where the id is assigned. Newly created instances of this"},{"line_number":1699,"context_line":"        # class, or instances loaded from legacy builder files that have no"},{"line_number":1700,"context_line":"        # persisted id, must be saved in order for an id to be assigned."},{"line_number":1701,"context_line":"        id_persisted \u003d True"},{"line_number":1702,"context_line":"        if self._id is None:"},{"line_number":1703,"context_line":"            id_persisted \u003d False"},{"line_number":1704,"context_line":"            self._id \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_12f9354c","line":1701,"in_reply_to":"5ff73747_ed03ac0c","updated":"2017-04-25 16:47:05.000000000","message":"I also pondered that :)\n\nand I flirted with:\n\n  current_id \u003d self._id\n  if self._id is None\n      self._id \u003d uuid.uuid4().hex\n  try:\n    ...\n  except Exception:\n      self._id \u003d current_id\n      raise\n\nI\u0027m happy with any, but when a reviewer comments then it\u0027s often a sign that code could be more intuitive","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":1707,"context_line":"                pickle.dump(self.to_dict(), f, protocol\u003d2)"},{"line_number":1708,"context_line":"        except Exception:"},{"line_number":1709,"context_line":"            if not id_persisted:"},{"line_number":1710,"context_line":"                self._id \u003d None"},{"line_number":1711,"context_line":"            raise"},{"line_number":1712,"context_line":""},{"line_number":1713,"context_line":"    def search_devs(self, search_values):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_051d5f0e","line":1710,"updated":"2017-04-26 03:37:58.000000000","message":"you guys thought of *everything* - KUDOS!\n\nsee test_id_not_initialised_errors","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"}],"swift/common/ring/composite_builder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":31,"context_line":")"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def pre_validate_all_builders(builders):"},{"line_number":35,"context_line":"    \"\"\""},{"line_number":36,"context_line":"    Pre-validation for all ring builders which is included into the new one"},{"line_number":37,"context_line":"    ring."}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_55128b63","line":34,"updated":"2017-03-17 18:43:42.000000000","message":"nit: let\u0027s be consistent in use of names - either \u0027builders\u0027 eveywhere or \u0027ring_builders\u0027 everywhere. I would vote for \u0027builders\u0027  - it is shorter :)","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":31,"context_line":")"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def pre_validate_all_builders(builders):"},{"line_number":35,"context_line":"    \"\"\""},{"line_number":36,"context_line":"    Pre-validation for all ring builders which is included into the new one"},{"line_number":37,"context_line":"    ring."}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_949e0461","line":34,"in_reply_to":"1a1ced50_55128b63","updated":"2017-03-23 11:01:43.000000000","message":"Done. Now all variable is \"builders\"","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":64,"context_line":"            #       different rings as different region. However, on that"},{"line_number":65,"context_line":"            #       case, what\u0027s the new region expression for the affinity?"},{"line_number":66,"context_line":"            #       To avoid the discussion, here simply raise validation"},{"line_number":67,"context_line":"            #       error"},{"line_number":68,"context_line":"            raise ValueError(\u0027Same region found in different rings\u0027)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_52919898","line":67,"updated":"2017-03-20 11:02:13.000000000","message":"I think this is OK for our first implementation. We are not preventing this constraint being relaxed in future if we find another way to differentiate between component rings.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":75,"context_line":"    :param ring_data: a RingData insatnce for the zipped ring"},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # To be simple, just the validate zipped ring here, it is impossible"},{"line_number":78,"context_line":"    # to report which ring files the incorrect devices appeared in though."},{"line_number":79,"context_line":"    ip_port_dev_set \u003d set()"},{"line_number":80,"context_line":"    for dev in ring_data.devs:"},{"line_number":81,"context_line":"        if (dev[\u0027ip\u0027], dev[\u0027port\u0027], dev[\u0027device\u0027]) in ip_port_dev_set:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_b522dfb7","line":78,"updated":"2017-03-17 18:43:42.000000000","message":"See my gist - not too hard, I think","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":75,"context_line":"    :param ring_data: a RingData insatnce for the zipped ring"},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # To be simple, just the validate zipped ring here, it is impossible"},{"line_number":78,"context_line":"    # to report which ring files the incorrect devices appeared in though."},{"line_number":79,"context_line":"    ip_port_dev_set \u003d set()"},{"line_number":80,"context_line":"    for dev in ring_data.devs:"},{"line_number":81,"context_line":"        if (dev[\u0027ip\u0027], dev[\u0027port\u0027], dev[\u0027device\u0027]) in ip_port_dev_set:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_34b2b0e5","line":78,"in_reply_to":"1a1ced50_b522dfb7","updated":"2017-03-23 11:01:43.000000000","message":"Yup, I like your diff, that will be included into the next patch set.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":87,"context_line":"            ip_port_dev_set.add((dev[\u0027ip\u0027], dev[\u0027port\u0027], dev[\u0027device\u0027]))"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"def composite(ring_builders):"},{"line_number":91,"context_line":"    \"\"\""},{"line_number":92,"context_line":"    :param ring_builders: a list of"},{"line_number":93,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder` instance"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_b0ad9533","line":90,"updated":"2017-03-17 18:43:42.000000000","message":"\"composite\" can be used as a verb but \"compose\" would be a more commonly used verb \n\nperhaps \n\n  compose_rings","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":87,"context_line":"            ip_port_dev_set.add((dev[\u0027ip\u0027], dev[\u0027port\u0027], dev[\u0027device\u0027]))"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"def composite(ring_builders):"},{"line_number":91,"context_line":"    \"\"\""},{"line_number":92,"context_line":"    :param ring_builders: a list of"},{"line_number":93,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder` instance"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_54d48cea","line":90,"in_reply_to":"1a1ced50_b0ad9533","updated":"2017-03-23 11:01:43.000000000","message":"Good idea, done.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":96,"context_line":"    \"\"\""},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    pre_validate_all_builders(ring_builders)"},{"line_number":99,"context_line":"    zipped_r2p2d \u003d []"},{"line_number":100,"context_line":"    zipped_devs \u003d []"},{"line_number":101,"context_line":"    composite_info \u003d []"},{"line_number":102,"context_line":"    device_offset \u003d 0"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_ccb6d189","line":99,"updated":"2017-03-20 11:02:13.000000000","message":"nit: might be helpful to have a single consistent term for the union of multiple rings: i.e. choose \u0027zipped\u0027 or \u0027composite\u0027 everywhere. I would vote for \u0027composite\u0027 because  we\u0027re not ever doing a python zip.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":96,"context_line":"    \"\"\""},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    pre_validate_all_builders(ring_builders)"},{"line_number":99,"context_line":"    zipped_r2p2d \u003d []"},{"line_number":100,"context_line":"    zipped_devs \u003d []"},{"line_number":101,"context_line":"    composite_info \u003d []"},{"line_number":102,"context_line":"    device_offset \u003d 0"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_1459348e","line":99,"in_reply_to":"1a1ced50_ccb6d189","updated":"2017-03-23 11:01:43.000000000","message":"Ok, that\u0027s from because we had called this as \"swift-ring-zipper\". we don\u0027t need to keep the word anymore.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        for part2dev in r2p2d:"},{"line_number":109,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":110,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":111,"context_line":"                devs[dev][\u0027id\u0027] \u003d part2dev[part]"},{"line_number":112,"context_line":"        zipped_r2p2d.extend(r2p2d)"},{"line_number":113,"context_line":"        zipped_devs.extend(devs)"},{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_394c2970","line":111,"updated":"2017-03-17 18:43:42.000000000","message":"devs can have holes i.e. devs[dev] may be None -see my gist","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        for part2dev in r2p2d:"},{"line_number":109,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":110,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":111,"context_line":"                devs[dev][\u0027id\u0027] \u003d part2dev[part]"},{"line_number":112,"context_line":"        zipped_r2p2d.extend(r2p2d)"},{"line_number":113,"context_line":"        zipped_devs.extend(devs)"},{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_ac4d159b","line":111,"in_reply_to":"1a1ced50_394c2970","updated":"2017-03-20 11:02:13.000000000","message":"nit: I found it more intuitive to read:\n\n                part2dev[part] +\u003d device_offset\n                devs[dev][\u0027id\u0027] +\u003d device_offset","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        for part2dev in r2p2d:"},{"line_number":109,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":110,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":111,"context_line":"                devs[dev][\u0027id\u0027] \u003d part2dev[part]"},{"line_number":112,"context_line":"        zipped_r2p2d.extend(r2p2d)"},{"line_number":113,"context_line":"        zipped_devs.extend(devs)"},{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_b47b00bd","line":111,"in_reply_to":"1a1ced50_ac4d159b","updated":"2017-03-23 11:01:43.000000000","message":"cool, done.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":113,"context_line":"        zipped_devs.extend(devs)"},{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_b73abe96","line":116,"updated":"2017-03-20 11:02:13.000000000","message":"If the intention is to use this md5 hash to validate that the same builder has been passed to a future operation, then this is quite brittle - any future addition to builder dict keys (for example to add some metadata) would change the md5 when the ring could in fact be the same.\n\nIs there some subset of the dict keys that we would expect to be immutable?\n\nOr, alternatively, could we assign an \"immutable\" UUID to a builder/ring?","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":113,"context_line":"        zipped_devs.extend(devs)"},{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_77f7cad2","line":116,"in_reply_to":"1a1ced50_b73abe96","updated":"2017-03-23 11:01:43.000000000","message":"I think immutable UUID one is too hard to make it robust because we already has a lot of swift clusters all over the world and the rings doesn\u0027t have the UUID. Even we can add the UUID somehow (e.g. next rebalance after upgrading?) that could be still inconsistent with the old ring.gz.\n\nThe idea, I added the md5 here, is for operator confirmation. For now we don\u0027t have any mapping like builder file (we\u0027re now discussing though) and it could be hard to know where of the composite ring consists. If we have the md5 here, it will make it easy to check the consistency between the composite ring and the parts.\n\nThinking of your comment, I\u0027m not so afraid the situation, operator changes the device metadata and the md5 will be changed, because it\u0027s actually different state in the ring builder. Exactly, we could enhance (select) the items to feed the md5 hash to avoid the md5 diff with small change though, it could be more complex than current simple implementation?\n\nIf we want to do that, maybe ring builder have the functionality\n\n @property\n def md5(self):\n    return md5(*serialized selected items*).hexdigest()\n\n\nthought?","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"0bce35cdb95866041e6ef3a5107a99b44a41fca0","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_44cafa50","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":34},"updated":"2017-03-17 05:03:03.000000000","message":"Probably more info like \u0027replica_count\u0027, \u0027region\u0027 would be useful in the future?","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"099da8293fc3bd15868921a63489a1335066b11f","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_591ba572","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":34},"in_reply_to":"1a1ced50_44cafa50","updated":"2017-03-17 18:43:42.000000000","message":"replica count is implied in ring file replica2part2dev2\n\nregions could be useful for rapid validation when adding new rings??\n\nbut should we have a \u0027builder file\u0027 for that metadata??","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_37d392d6","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":34},"in_reply_to":"1a1ced50_4c302102","updated":"2017-03-23 11:01:43.000000000","message":"Yes, replic_count per component ring.\n\nBut not yet addressed on my comment in the next version.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":115,"context_line":"        builder_hash \u003d md5("},{"line_number":116,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":117,"context_line":"        composite_info.append({"},{"line_number":118,"context_line":"            \u0027builder_file\u0027: builder.builder_file,"},{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_4c302102","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":34},"in_reply_to":"1a1ced50_591ba572","updated":"2017-03-20 11:02:13.000000000","message":"OIC, replica_count per component ring *does* make a lot sense, ignore my previous comment.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f0ffc1ebedd0a7c7915c8817fd371efb5027e4ce","unresolved":false,"context_lines":[{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"},{"line_number":123,"context_line":"    validate_dev_uniqueness(rd)"},{"line_number":124,"context_line":"    return rd"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_f738b63f","line":122,"updated":"2017-03-20 11:02:13.000000000","message":"Hmmm, The composite_info is not used in this patch, and we\u0027re not yet sure how it would be used...should we reverse the order of the patch chain? So this patch adds this module, then next patch adds support to save composite_info to ring as metadata?","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"fa1780862c7172e8ef9225a33ef9589c169f7218","unresolved":false,"context_lines":[{"line_number":119,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    rd \u003d RingData(zipped_r2p2d, zipped_devs, ring_builders[0].part_shift,"},{"line_number":122,"context_line":"                  {\u0027composite_info\u0027: composite_info})"},{"line_number":123,"context_line":"    validate_dev_uniqueness(rd)"},{"line_number":124,"context_line":"    return rd"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a1ced50_daeb4311","line":122,"in_reply_to":"1a1ced50_f738b63f","updated":"2017-03-23 11:01:43.000000000","message":"Ah, yes. This is not used right now. I just preserved it for future improvement. (which may be useful?)\n\nOnce we had the metadata here, in the future, we could use the info on either the new CLI tool (in particular validation whatever) or the proxy-server handling (e.g. node election or selecting handoffs). Exactly, this may be a thing of YAGNI but, as well as UUID at builder file discussion, once the ring built (or deployed), adding the new information could be hard problem.\n\nAnd, at least, I think this is useful for operators even at this time to confirm the rings inside the composite ring by hand.","commit_id":"99cb057993d78350665ad74f5969cac805d8d23d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5ca4a467fedac4160ae8e716188c115f9dea3880","unresolved":false,"context_lines":[{"line_number":124,"context_line":"        composite_devs.extend(devs)"},{"line_number":125,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":126,"context_line":"        builder_hash \u003d md5("},{"line_number":127,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":128,"context_line":"        composite_info.append({"},{"line_number":129,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":130,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a1ced50_c292674b","line":127,"updated":"2017-03-23 20:31:34.000000000","message":"my thinking in previous comment w.r.t. introducing a UUID for each builder is that when opening a builder that has no uuid then we can add one and save. That way legacy rings can be given UUIDs as we use them for composites.\n\nI\u0027m not sure I understand the goal with using the md5 of *all* the builder attributes - that includes the replica2part2dev so the md5 of same but rebalance component ring won\u0027t match the md5 of a component in composite ring? So they would prevent any update of existing components?","commit_id":"b727abd928f1bcfa9e76075ec11495905a5a5182"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"f2d108c6c117c476776fc9465eb6ba1eba03964b","unresolved":false,"context_lines":[{"line_number":124,"context_line":"        composite_devs.extend(devs)"},{"line_number":125,"context_line":"        device_offset +\u003d len(builder.devs)"},{"line_number":126,"context_line":"        builder_hash \u003d md5("},{"line_number":127,"context_line":"            pickle.dumps(builder.to_dict(), protocol\u003d2)).hexdigest()"},{"line_number":128,"context_line":"        composite_info.append({"},{"line_number":129,"context_line":"            \u0027hash\u0027: builder_hash})"},{"line_number":130,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ffe62b97_4e5e30e7","line":127,"in_reply_to":"1a1ced50_c292674b","updated":"2017-03-24 09:46:27.000000000","message":"Basically, the reason I\u0027d care of the md5sum is for the traceability of the component rings. In the etherpad discussion[1], I think, we\u0027re avoiding to import all the component ring info (like devs list and part2dev table) into the composite ring builder file. (See CLI tool section Idea 3) Instead, the CLI tool will maintain the path to each ring builder file.\n\nCurrently, I\u0027m feeling it\u0027s brittle because the actual builder files pointed from the composite builder can be updated independently anytime and the CLI tool won\u0027t (cannot, maybe?) maintain the state of the ring builders without extra info like md5 or version number or...whatever. So that I\u0027d like to add the identifier of the component rings which means \"this composite ring built from those builders\".\n\nAnd then, the md5sum of the to_dict() result seemed safe to express the builder statuses.\n\n\n\nI think the discussion point is which level we *want* to guarantee the ring status, if we don\u0027t want to the state with the level of the part2dev mapping, just uuid of the ring builder file will be the solution. On the other hand, if we care about the par2dev table, the uuid cannot care of it, we will need md5 of the dict. (or just whole body of the serialized builder data on disk!?)\n\nThe reason why I prefer to the md5 (trace all status of the builder files) is we should validate at least the devs list (device uniqueness) at the composite timing so imagine the situation after employing \"uuid\" for the builder files, if the operator still uses *correct builder files* with *correct uuids* but the compose operation could be going to fail when the operator updates the component ring builder files (e.g. add same dev/ip/port) even the uuids verified as correct.\n\nThat could be overthinking...though.\n\n1. https://etherpad.openstack.org/p/composite_rings\n\n(since reviewing your follow up patch and after trying some more experimental tests)\n\nMy intention above may be broken, probably my misunderstanding of pickle mechanism. At that time, I thought that the md5 value of the dict should be stable unless the ring builder changed (e.g. add dev, remove dev, rebalance... etc) but I found the hash of to_dict() can be changed when just the builder was written to the disk [2]. It seems from a pickle problem (pickle serialization for dict instances could result in the serialized binary difference). If we would attempt to avoid it, probably we need to consider other format like json instead of pickle, which may be painful.\n\nConsidering that, the md5 is not needed and not good choice anymore because that is not able to be used for the assertion. Instead, I\u0027m realizing some combination of info like uuid + builder version can be used for the identifier, right now. How do you think of it?\n\n2: https://gist.github.com/bloodeagle40234/af7e400c5df9da0fb2f2ddd8ed2b73bd","commit_id":"b727abd928f1bcfa9e76075ec11495905a5a5182"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"247c9f2719c31ba45de43824105d72e630af6ac1","unresolved":false,"context_lines":[{"line_number":140,"context_line":""},{"line_number":141,"context_line":"def _make_component_meta(builder):"},{"line_number":142,"context_line":"    # return a dict of selected builder attributes to save in composite meta"},{"line_number":143,"context_line":"    attrs \u003d [\u0027version\u0027, \u0027replicas\u0027, \u0027source\u0027]"},{"line_number":144,"context_line":"    metadata \u003d dict((attr, getattr(builder, attr)) for attr in attrs)"},{"line_number":145,"context_line":"    return metadata"},{"line_number":146,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_3873cf4f","line":143,"updated":"2017-03-27 16:02:09.000000000","message":"is it useful to include regions? as a shortcut to save iterating over the devs","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"247c9f2719c31ba45de43824105d72e630af6ac1","unresolved":false,"context_lines":[{"line_number":148,"context_line":"def _make_composite_metadata(builders):"},{"line_number":149,"context_line":"    # return a list of component builder metadata"},{"line_number":150,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":151,"context_line":"    return {\u0027components\u0027: component_meta}"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_58ad7386","line":151,"updated":"2017-03-27 16:02:09.000000000","message":"add a version number!","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04ce25c11ad1e4289b6fc1ce7fab4b8788f24a97","unresolved":false,"context_lines":[{"line_number":148,"context_line":"def _make_composite_metadata(builders):"},{"line_number":149,"context_line":"    # return a list of component builder metadata"},{"line_number":150,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":151,"context_line":"    return {\u0027components\u0027: component_meta}"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_7475c231","line":151,"in_reply_to":"ffe62b97_58ad7386","updated":"2017-03-28 08:16:15.000000000","message":"Love it!","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"79166fe2830d15df2351c24c4fad651de7eeee30","unresolved":false,"context_lines":[{"line_number":148,"context_line":"def _make_composite_metadata(builders):"},{"line_number":149,"context_line":"    # return a list of component builder metadata"},{"line_number":150,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":151,"context_line":"    return {\u0027components\u0027: component_meta}"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_80459945","line":151,"in_reply_to":"ffe62b97_7475c231","updated":"2017-03-28 10:37:24.000000000","message":"Done","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"247c9f2719c31ba45de43824105d72e630af6ac1","unresolved":false,"context_lines":[{"line_number":165,"context_line":"    by the old_component"},{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_58ca136d","line":168,"updated":"2017-03-27 16:02:09.000000000","message":"we\u0027ll need to change this to inspect only the relative filename rather than absolute path, or have the builder only save the filename in source","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04ce25c11ad1e4289b6fc1ce7fab4b8788f24a97","unresolved":false,"context_lines":[{"line_number":165,"context_line":"    by the old_component"},{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_94960693","line":168,"in_reply_to":"ffe62b97_58ca136d","updated":"2017-03-28 08:16:15.000000000","message":"ok, check_same_builder is used in check_against_existing so we need some identifiers to compare the ring builders (the new one and the existing one.)\n\nI think you intend to keep the patch order as existing (not uuid first), so here, we just validate the builder file path.","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"79166fe2830d15df2351c24c4fad651de7eeee30","unresolved":false,"context_lines":[{"line_number":165,"context_line":"    by the old_component"},{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_2720279b","line":168,"in_reply_to":"ffe62b97_94960693","updated":"2017-03-28 10:37:24.000000000","message":"Kota and I discussed this in IRC. For now we will leave this as it is i.e. composite_builder will simply check that the \u0027source\u0027 of each component builder has not changed. composite_buider will NOT parse \u0027source\u0027 further to infer basenames.\n\nWhatever calls into composite_builder with a list of builders should be responsible for ensuring that the builder sources take consistent and useful values. That could be a consistent relative path, basename etc.","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04ce25c11ad1e4289b6fc1ce7fab4b8788f24a97","unresolved":false,"context_lines":[{"line_number":207,"context_line":"    old_components \u003d old_composite_meta[\u0027components\u0027]"},{"line_number":208,"context_line":"    new_components \u003d new_composite_meta[\u0027components\u0027]"},{"line_number":209,"context_line":"    for i, old_component in enumerate(old_components):"},{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            new_component \u003d new_components[i]"},{"line_number":212,"context_line":"        except IndexError:"},{"line_number":213,"context_line":"            errors.append(\"Missing builder at index %d\" % i)"},{"line_number":214,"context_line":"            continue"},{"line_number":215,"context_line":"        try:"},{"line_number":216,"context_line":"            # check we have same component builder in this position vs existing"},{"line_number":217,"context_line":"            check_same_builder(old_component, new_component)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_14531641","line":214,"range":{"start_line":210,"start_character":0,"end_line":214,"end_character":20},"updated":"2017-03-28 08:16:15.000000000","message":"I\u0027d like to be careful here and at L224.\n\nReading code as written, are those item correct?\n\n- if the new component shorter than old component, that\u0027s ok i.e. we can remove a (or some) builder(s) in the new_component\n\n- if we add some rings into the new component, it will fail as ValueError, so we cannot add any rings once the existing composite ring build over there.\n\nif those are correct, it sounds inconsistent with what we want. In the future operation, we may want to add a new ring into the composite ring to add a new region? but this code doesn\u0027t allow it. Try to make the test...","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"58993016df2bb34633096212136a9cc5eec3930c","unresolved":false,"context_lines":[{"line_number":207,"context_line":"    old_components \u003d old_composite_meta[\u0027components\u0027]"},{"line_number":208,"context_line":"    new_components \u003d new_composite_meta[\u0027components\u0027]"},{"line_number":209,"context_line":"    for i, old_component in enumerate(old_components):"},{"line_number":210,"context_line":"        try:"},{"line_number":211,"context_line":"            new_component \u003d new_components[i]"},{"line_number":212,"context_line":"        except IndexError:"},{"line_number":213,"context_line":"            errors.append(\"Missing builder at index %d\" % i)"},{"line_number":214,"context_line":"            continue"},{"line_number":215,"context_line":"        try:"},{"line_number":216,"context_line":"            # check we have same component builder in this position vs existing"},{"line_number":217,"context_line":"            check_same_builder(old_component, new_component)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_e7fa3fbb","line":214,"range":{"start_line":210,"start_character":0,"end_line":214,"end_character":20},"in_reply_to":"ffe62b97_14531641","updated":"2017-03-28 09:17:12.000000000","message":"The first item is false. Because if IndexError found, it will be appended to the errors list so this makes us strict constraint the length should not be different. Make sense.\n\nAnd hopefully, if you want to add/remove rings, the functionality will be cared by CLI tool or just set a *new ring name* in the write_composite_ring function. It sounds simple.","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04ce25c11ad1e4289b6fc1ce7fab4b8788f24a97","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        try:"},{"line_number":216,"context_line":"            # check we have same component builder in this position vs existing"},{"line_number":217,"context_line":"            check_same_builder(old_component, new_component)"},{"line_number":218,"context_line":"            modified |\u003d is_builder_modified(old_component, new_component)"},{"line_number":219,"context_line":"        except ValueError as err:"},{"line_number":220,"context_line":"            errors.append(\"Unexpected builder at index %d: \""},{"line_number":221,"context_line":"                          \"%r does not match existing %r: %s\" %"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_d4dfee37","line":218,"range":{"start_line":218,"start_character":12,"end_line":218,"end_character":73},"updated":"2017-03-28 08:16:15.000000000","message":"And then, also checking the builder version, here...\n\nAh, ok. the increasing the version number is not minded because this works to prevent the redundant composite operation if nothing changed.","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7c860f3d6d07077da84600cbf295519cc1849147","unresolved":false,"context_lines":[{"line_number":236,"context_line":"    metadata. If the composite ring file already exists then verify that the"},{"line_number":237,"context_line":"    list of builders is consistent with the existing composition of builders."},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"    :param builder_files: a list instances of"},{"line_number":240,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder`"},{"line_number":241,"context_line":"    :param composite_ring_file: absolute path to ring file to be written"},{"line_number":242,"context_line":"    :raises: ValueError if the component ring builders are not suitable for"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_f1258bac","line":239,"updated":"2017-03-27 13:31:57.000000000","message":"s/builder_files/builders/","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"79166fe2830d15df2351c24c4fad651de7eeee30","unresolved":false,"context_lines":[{"line_number":236,"context_line":"    metadata. If the composite ring file already exists then verify that the"},{"line_number":237,"context_line":"    list of builders is consistent with the existing composition of builders."},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"    :param builder_files: a list instances of"},{"line_number":240,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder`"},{"line_number":241,"context_line":"    :param composite_ring_file: absolute path to ring file to be written"},{"line_number":242,"context_line":"    :raises: ValueError if the component ring builders are not suitable for"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_6078558f","line":239,"in_reply_to":"ffe62b97_f1258bac","updated":"2017-03-28 10:37:24.000000000","message":"Done","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"79166fe2830d15df2351c24c4fad651de7eeee30","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        # to avoid inadvertently mangling an existing composite ring, load it"},{"line_number":250,"context_line":"        # and check that the current list of builders is consistent with those"},{"line_number":251,"context_line":"        # use to compose the existing ring"},{"line_number":252,"context_line":"        rd \u003d RingData.load(composite_ring_file)"},{"line_number":253,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":254,"context_line":"        if old_composite_meta is None:"},{"line_number":255,"context_line":"            raise ValueError(\"Not a composite ring file: %s\" %"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_a01f7db5","line":252,"updated":"2017-03-28 10:37:24.000000000","message":"use metadata_only flag when loading old ring","commit_id":"598159a35dfe6f104c7d74b428a097aec072214a"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"8d9c15abf756f4a4a742a9751550f799e04a5f5a","unresolved":false,"context_lines":[{"line_number":148,"context_line":"def _make_composite_metadata(builders):"},{"line_number":149,"context_line":"    # return a list of component builder metadata"},{"line_number":150,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":151,"context_line":"    return {\u0027version\u0027: 1, \u0027components\u0027: component_meta}"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ffe62b97_4a7aebb9","line":151,"range":{"start_line":151,"start_character":13,"end_line":151,"end_character":24},"updated":"2017-03-29 01:41:40.000000000","message":"will it be consistent with some CLI tools version number, you intended rather than fixed version 1, right?","commit_id":"2ffc288e26e76fd74b13ce171e646de25409532f"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"8d9c15abf756f4a4a742a9751550f799e04a5f5a","unresolved":false,"context_lines":[{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"def is_builder_modified(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ffe62b97_4da56546","line":170,"range":{"start_line":169,"start_character":0,"end_line":170,"end_character":71},"updated":"2017-03-29 01:41:40.000000000","message":"Ok, this is cared by the caller function (i.e. check_against_existing) to say \"the metadata is different\" and it\u0027 s asserted in unit tests but currently this message is not asserted even you added the error into the error lines at L222. Once I remove the message here, no unit test fails. Will update the unit test to assert such a worth lines included in.","commit_id":"2ffc288e26e76fd74b13ce171e646de25409532f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2789655dc60ccafbc2cc11c963501ef02deed647","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        for part2dev in r2p2d:"},{"line_number":115,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":116,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":117,"context_line":"                devs[dev][\u0027id\u0027] +\u003d device_offset"},{"line_number":118,"context_line":"        composite_r2p2d.extend(r2p2d)"},{"line_number":119,"context_line":"        composite_devs.extend(devs)"},{"line_number":120,"context_line":"        device_offset +\u003d len(builder.devs)"}],"source_content_type":"text/x-python","patch_set":8,"id":"ffe62b97_5506ea53","line":117,"updated":"2017-03-29 17:00:12.000000000","message":"argh - dev id has device_offset added for every part. This is my fault, I suggested this change :/","commit_id":"104b2cb6c6d5447fbf835cb8b2df8b29f815277e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":128,"context_line":"    builders and return a composite RingData instance."},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    :param builders: a list of"},{"line_number":131,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder` instance"},{"line_number":132,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":133,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":134,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_43c5885a","line":131,"updated":"2017-04-04 03:30:00.000000000","message":"this +100","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":128,"context_line":"    builders and return a composite RingData instance."},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    :param builders: a list of"},{"line_number":131,"context_line":"        `swift.common.ring.ringbuilder.RingBuilder` instance"},{"line_number":132,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":133,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":134,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_d77032a8","line":131,"in_reply_to":"dfeb2761_43c5885a","updated":"2017-04-04 08:53:13.000000000","message":"Done","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":132,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":133,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":134,"context_line":"    \"\"\""},{"line_number":135,"context_line":"    pre_validate_all_builders(builders)"},{"line_number":136,"context_line":"    check_for_dev_uniqueness(builders)"},{"line_number":137,"context_line":"    rd \u003d _make_composite_ring(builders)"},{"line_number":138,"context_line":"    return rd"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_e3ab7c86","line":135,"updated":"2017-04-04 03:30:00.000000000","message":"I like that the crucial correct validation (e.g. part power) happens inside compose ring - while the \"safety\" validation stuff is mostly in optional helpers.  Kudos!","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":132,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":133,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":134,"context_line":"    \"\"\""},{"line_number":135,"context_line":"    pre_validate_all_builders(builders)"},{"line_number":136,"context_line":"    check_for_dev_uniqueness(builders)"},{"line_number":137,"context_line":"    rd \u003d _make_composite_ring(builders)"},{"line_number":138,"context_line":"    return rd"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_7765e6e0","line":135,"in_reply_to":"dfeb2761_e3ab7c86","updated":"2017-04-04 08:53:13.000000000","message":"Done","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":140,"context_line":""},{"line_number":141,"context_line":"def _make_component_meta(builder):"},{"line_number":142,"context_line":"    # return a dict of selected builder attributes to save in composite meta"},{"line_number":143,"context_line":"    attrs \u003d [\u0027version\u0027, \u0027replicas\u0027, \u0027source\u0027]"},{"line_number":144,"context_line":"    metadata \u003d dict((attr, getattr(builder, attr)) for attr in attrs)"},{"line_number":145,"context_line":"    return metadata"},{"line_number":146,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_836f1091","line":143,"updated":"2017-04-04 03:30:00.000000000","message":"How do we want to deal with replicas in check_same_builder?  In the EC case a change here could be pretty bad?\n\nI think we should just require replicas be the same - if we have a use-case to increase them later we can either support it with --force or think of a reasonable way to lift that restriction to a warning.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":140,"context_line":""},{"line_number":141,"context_line":"def _make_component_meta(builder):"},{"line_number":142,"context_line":"    # return a dict of selected builder attributes to save in composite meta"},{"line_number":143,"context_line":"    attrs \u003d [\u0027version\u0027, \u0027replicas\u0027, \u0027source\u0027]"},{"line_number":144,"context_line":"    metadata \u003d dict((attr, getattr(builder, attr)) for attr in attrs)"},{"line_number":145,"context_line":"    return metadata"},{"line_number":146,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_d741520d","line":143,"in_reply_to":"dfeb2761_836f1091","updated":"2017-04-04 08:53:13.000000000","message":"I think the \"replicas\" is not for check_same_builder but the future utility for Ring instance (not a Builder). You know, the composite ring should work as well as classical ring so that proxy-server is not able to find where part is built from which ring for now. If we have replicas in the ring meta, the proxy server can find the part (e.g. the first 3 replicas are from Ring0, and the next is from Ring1).\n\nThough, we don\u0027t decide yet the composite ring restriction for regions (for now, each ring should have unique regions each other) . If we keep the restriction, proxy can find the state via region info from each node but if we could want to mitigate the restriction, no one can collect the component info with Ring instance. e.g. affinity depends on component Rings??? (But... at that time, perhaps, do we want to care about component ring layout anymore???)\n\nHmmm....no idea for now, need to consider deeply.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"def is_builder_modified(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_03d78025","line":170,"updated":"2017-04-04 03:30:00.000000000","message":"this is *super* sketchy.  We need to at least normalize the path... \n\n    ValueError: Unexpected builder at index 0: {\u0027source\u0027: \u0027/home/ubuntu/region1.builder\u0027, \u0027version\u0027: 14, \u0027replicas\u0027: 6.0} does not match existing {u\u0027source\u0027: u\u0027region1.builder\u0027, u\u0027version\u0027: 13, u\u0027replicas\u0027: 6.0}: Attribute mismatch for source: u\u0027region1.builder\u0027 !\u003d \u0027/home/ubuntu/region1.builder\u0027\n    Unexpected builder at index 1: {\u0027source\u0027: \u0027/home/ubuntu/region2.builder\u0027, \u0027version\u0027: 9, \u0027replicas\u0027: 6.0} does not match existing {u\u0027source\u0027: u\u0027region2.builder\u0027, u\u0027version\u0027: 9, u\u0027replicas\u0027: 6.0}: Attribute mismatch for source: u\u0027region2.builder\u0027 !\u003d \u0027/home/ubuntu/region2.builder\u0027\n\nregion1.builder *is* /home/ubuntu/region1.builder !\n\nAnd what if you rename the files?  For a long time my builder has the same version # - when I flipped around the names - no one cared that I screwed up the order.\n\nWorse *I* still have remember the order.  Once I commit the order to disk in a shell script:\n\n    ring-zipper.py region1.builder region2.builder --output zipper-test.ring.gz\n\nHow much is really buying me that the tool makes sure my script hasn\u0027t changed?","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"def is_builder_modified(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_bdb58dcf","line":170,"in_reply_to":"dfeb2761_03d78025","updated":"2017-04-04 08:53:13.000000000","message":"I think Alistair is thinking we should replace the ring builder identifier from the path to the uuid in the future work so that currently we don\u0027t normalize here, just you should care of the path format (relative vs absolute).\n\nAnd I thought it may be ok for now.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":167,"context_line":"    key \u003d \u0027source\u0027"},{"line_number":168,"context_line":"    if old_component[key] !\u003d new_component[key]:"},{"line_number":169,"context_line":"        raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":170,"context_line":"                         (key, old_component[key], new_component[key]))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"def is_builder_modified(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_62799d2d","line":170,"in_reply_to":"dfeb2761_bdb58dcf","updated":"2017-04-04 11:20:04.000000000","message":"OK, so we need to iterate on builder ID/source etc\n\n@Clay the order is stored in the composite meta. In context of your other comments I can see that may not help you because that meta is currently in composite-ring.gz which you may not want to keep local).","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":184,"context_line":"    # than version, because version is incremented when devs are just added or"},{"line_number":185,"context_line":"    # removed. So we may want another builder flag for rebalance-specific"},{"line_number":186,"context_line":"    # version, or an md5 of replica2part2dev??"},{"line_number":187,"context_line":"    return old_component[\u0027version\u0027] !\u003d new_component[\u0027version\u0027]"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"def check_against_existing(old_composite_meta, new_composite_meta):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_237724a3","line":187,"updated":"2017-04-04 03:30:00.000000000","message":"version is incremented on rebalance as well - even if you just --force and nothing changes.\n\nIt\u0027s not incremented when you pretend_min_part_hours_passed, set_overload or set_replicas -  but none of those effect replica2part2dev until rebalance.  Oddly it *is* incremented for set_dev_weight \u003cshrug\u003e\n\nI think this *is* sufficient.  Maybe weird that you can go back to old versions.  But that\u0027s probably useful in some cases too... might nice to get a warning or something?","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":184,"context_line":"    # than version, because version is incremented when devs are just added or"},{"line_number":185,"context_line":"    # removed. So we may want another builder flag for rebalance-specific"},{"line_number":186,"context_line":"    # version, or an md5 of replica2part2dev??"},{"line_number":187,"context_line":"    return old_component[\u0027version\u0027] !\u003d new_component[\u0027version\u0027]"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"def check_against_existing(old_composite_meta, new_composite_meta):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_5d50c15d","line":187,"in_reply_to":"dfeb2761_237724a3","updated":"2017-04-04 08:53:13.000000000","message":"I think the builder increments the version when either replica2part2dev or devs list was changed and set_dev_weight will change the state at the devs list (even it\u0027s not affected to the actual device mapping until the next rebalance)\n\nCurrently this is used only to check to prevent a redundant work (i.e. if no version difference found here)... Ah, you mean, we should do like:\n\n old_component[\u0027version\u0027] \u003c new_component[\u0027version\u0027]\n\nthat means we should use newer ring always?","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":184,"context_line":"    # than version, because version is incremented when devs are just added or"},{"line_number":185,"context_line":"    # removed. So we may want another builder flag for rebalance-specific"},{"line_number":186,"context_line":"    # version, or an md5 of replica2part2dev??"},{"line_number":187,"context_line":"    return old_component[\u0027version\u0027] !\u003d new_component[\u0027version\u0027]"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"def check_against_existing(old_composite_meta, new_composite_meta):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_e098051c","line":187,"in_reply_to":"dfeb2761_5d50c15d","updated":"2017-04-04 11:20:04.000000000","message":"+1 for using \u003c\n\nre. version being sufficient, if the goal here is to only bother composing if the component replica2part2dev has changed, then testing for increased version is not sufficient because a device may have been added, but no rebalance done, so replica2part2dev has not changed when version has changed.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0a4e94e303c2833c0df91170ddcdd2189a161dfb","unresolved":false,"context_lines":[{"line_number":184,"context_line":"    # than version, because version is incremented when devs are just added or"},{"line_number":185,"context_line":"    # removed. So we may want another builder flag for rebalance-specific"},{"line_number":186,"context_line":"    # version, or an md5 of replica2part2dev??"},{"line_number":187,"context_line":"    return old_component[\u0027version\u0027] !\u003d new_component[\u0027version\u0027]"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"def check_against_existing(old_composite_meta, new_composite_meta):"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_62298699","line":187,"in_reply_to":"dfeb2761_e098051c","updated":"2017-04-05 19:45:42.000000000","message":"this is mostly a consequence of using builders instead of rings to compose (it\u0027s nice that we get context but we have to be careful)\n\nI *think* some set of attributes in the builder state gets\u0027 reset after a rebalance that can tell us if version is incremented because \n\n        self.devs_changed \u003d False\n\nat the bottom of rebalance.  I think maybe we should ValueError on rings that have devs_changed \u003d True and suggest rebalance first?","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":219,"context_line":"        except ValueError as err:"},{"line_number":220,"context_line":"            errors.append(\"Unexpected builder at index %d: \""},{"line_number":221,"context_line":"                          \"%r does not match existing %r: %s\" %"},{"line_number":222,"context_line":"                          (i, new_component, old_component, err))"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    for j, new_component in enumerate(new_components[i + 1:], start\u003di + 1):"},{"line_number":225,"context_line":"        errors.append(\"Unexpected extra builder at index %d: %r\" %"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_2dd71426","line":222,"updated":"2017-04-04 03:30:00.000000000","message":"This doesn\u0027t seem correct to me.  Why can\u0027t I have a cluster with 3 replicated policies:\n\n  1. region #1\n  2. region #2\n  3. global (composite of region #1 and region #2)\n\nAnd then grow a third region and have 4 policies:\n\n  1. region #1\n  2. region #2\n  3. global (composite of region #1 and region #2)\n  4. region #3\n\nIt seems like I *could* do that with this change - the suggestion would be delete objects-3.ring.gz before create a new builder from [r1, r2, r3]?\n\nWhere\u0027s the safety in that?  If the metadata that we\u0027re stuffing into the ring was exposed somewhere that I could edit it - it would seem safe and easy to add a new ring and preserve order.\n\nGlobal EC in this case requires change in ec_duplication_factor - so I\u0027d need to coordinate the swift.conf reload with the ring rollout (which could be a problem)","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":219,"context_line":"        except ValueError as err:"},{"line_number":220,"context_line":"            errors.append(\"Unexpected builder at index %d: \""},{"line_number":221,"context_line":"                          \"%r does not match existing %r: %s\" %"},{"line_number":222,"context_line":"                          (i, new_component, old_component, err))"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    for j, new_component in enumerate(new_components[i + 1:], start\u003di + 1):"},{"line_number":225,"context_line":"        errors.append(\"Unexpected extra builder at index %d: %r\" %"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_fb1438a2","line":222,"in_reply_to":"dfeb2761_2dd71426","updated":"2017-04-04 11:20:04.000000000","message":"Not sure I understand - your example has same composite ring before and after adding a fourth policy - did you mean to add a ring to the composite? I did not support adding rings *yet* because I wanted to keep this as simple as possible - if we support adding rings, do we also support removing rings?\n\nThe best suggestion for supporting those use cases would be to patch this code, not delete ring files. I\u0027m not saying that is hard to do - but it is less simple for this first step. Or we implement that right now.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":219,"context_line":"        except ValueError as err:"},{"line_number":220,"context_line":"            errors.append(\"Unexpected builder at index %d: \""},{"line_number":221,"context_line":"                          \"%r does not match existing %r: %s\" %"},{"line_number":222,"context_line":"                          (i, new_component, old_component, err))"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    for j, new_component in enumerate(new_components[i + 1:], start\u003di + 1):"},{"line_number":225,"context_line":"        errors.append(\"Unexpected extra builder at index %d: %r\" %"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_f8d56344","line":222,"in_reply_to":"dfeb2761_2dd71426","updated":"2017-04-04 08:53:13.000000000","message":"Probably that\u0027s what I thought in the old review[1], exactly current code disallows to add a new ring builder to existing one *BUT* we *CAN* create a new ring (with the new ring name) from [r1, r2, r3] builders. (or deleting existing object-3.ring.gz as you described)\n\nAnyway, I think this *check_against_exisiting* is for continuous ring integration (add/delete dev, rebalance, add/delete dev, rebalance...) on the composite ring and the new definition of the ring (i.e. change [r1, r2] to [r1, r2, r3] is not in the scope.\n\nI think that why Alistair designed this method as this is. And I think the definition change will be maintained in the CLI tool or a class something like CompositeRingBuilder?\n\n1: https://review.openstack.org/#/c/441921/6/swift/common/ring/composite_builder.py@214","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0a4e94e303c2833c0df91170ddcdd2189a161dfb","unresolved":false,"context_lines":[{"line_number":219,"context_line":"        except ValueError as err:"},{"line_number":220,"context_line":"            errors.append(\"Unexpected builder at index %d: \""},{"line_number":221,"context_line":"                          \"%r does not match existing %r: %s\" %"},{"line_number":222,"context_line":"                          (i, new_component, old_component, err))"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    for j, new_component in enumerate(new_components[i + 1:], start\u003di + 1):"},{"line_number":225,"context_line":"        errors.append(\"Unexpected extra builder at index %d: %r\" %"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_a2b0ae8b","line":222,"in_reply_to":"dfeb2761_fb1438a2","updated":"2017-04-05 19:45:42.000000000","message":"ok, i\u0027m fine with out of scope - thanks","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                                          new_composite_meta)"},{"line_number":259,"context_line":"        if not modified:"},{"line_number":260,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":261,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":264,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_03c0a04f","line":261,"updated":"2017-04-04 03:30:00.000000000","message":"If a CLI wanted to add a --force flag I guess it would just skip this entire block...","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                                          new_composite_meta)"},{"line_number":259,"context_line":"        if not modified:"},{"line_number":260,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":261,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":264,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_cc0e608c","line":261,"in_reply_to":"dfeb2761_03c0a04f","updated":"2017-04-04 08:53:13.000000000","message":"Sounds reasonable but I\u0027m wondering how we can detect that. should we compare all replica2part2devs table for each time?","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                                          new_composite_meta)"},{"line_number":259,"context_line":"        if not modified:"},{"line_number":260,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":261,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":264,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_1b4b5cb0","line":261,"in_reply_to":"dfeb2761_03c0a04f","updated":"2017-04-04 11:20:04.000000000","message":"yes","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"63de9b959ee15b1ad8f9d616a74a65e6d292bd00","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                                          new_composite_meta)"},{"line_number":259,"context_line":"        if not modified:"},{"line_number":260,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":261,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":264,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_336f44fd","line":261,"in_reply_to":"dfeb2761_1b4b5cb0","updated":"2017-04-05 07:27:05.000000000","message":"ok","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":132,"context_line":"        for dev in [d for d in devs if d]:"},{"line_number":133,"context_line":"            # note that some devs may not be referenced in r2p2d but update"},{"line_number":134,"context_line":"            # their dev id nonetheless"},{"line_number":135,"context_line":"            dev[\u0027id\u0027] +\u003d device_offset"},{"line_number":136,"context_line":"        composite_r2p2d.extend(r2p2d)"},{"line_number":137,"context_line":"        composite_devs.extend(devs)"},{"line_number":138,"context_line":"        device_offset +\u003d len(builder.devs)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_4f3c272e","line":135,"updated":"2017-04-12 05:20:37.000000000","message":"Er - Got it. so the old code updates only the dev ids appeared in r2p2dev table and if some devices which not assigned to the table can hold the id in the original builder that can conflict with other builders.\n\nIt looks good catch to me.","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":41,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":42,"context_line":"    \"\"\""},{"line_number":43,"context_line":"    if len(builders) \u003c 2:"},{"line_number":44,"context_line":"        raise ValueError(\u0027Two or more component builders are required.\u0027)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    # all ring builders should be consistent for each MUST_MATCH_ATTRS"},{"line_number":47,"context_line":"    for attr in MUST_MATCH_ATTRS:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_4de4e0ac","line":44,"updated":"2017-04-24 21:37:25.000000000","message":"I was debating about whether there ought to be some sort of maximum (in part because of the use of itertools.combinations), but\n\n- I suppose it will become unwieldy to users well before any other issue comes up, and\n- combinations(..., 2) can only grow like N**2 anyway. Not great, but not N!.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":41,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":42,"context_line":"    \"\"\""},{"line_number":43,"context_line":"    if len(builders) \u003c 2:"},{"line_number":44,"context_line":"        raise ValueError(\u0027Two or more component builders are required.\u0027)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    # all ring builders should be consistent for each MUST_MATCH_ATTRS"},{"line_number":47,"context_line":"    for attr in MUST_MATCH_ATTRS:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_1b2284a5","line":44,"in_reply_to":"5ff73747_4de4e0ac","updated":"2017-04-25 08:45:31.000000000","message":"Hmm... I didn\u0027t think about the max value for now.\n\nCurrently Alistair is working on the CompositeRingBuilder class so that we could have some more options or restriction at the caller layer, maybe?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                \u0027All builders must have same value for %r.\\n%s\u0027"},{"line_number":58,"context_line":"                % (attr, \u0027\\n  \u0027.join(variations)))"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    # all ring builders should have int replica count and not have dirty mods"},{"line_number":61,"context_line":"    errors \u003d []"},{"line_number":62,"context_line":"    for index, builder in enumerate(builders):"},{"line_number":63,"context_line":"        if int(builder.replicas) !\u003d builder.replicas:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_adb6c4d8","line":60,"updated":"2017-04-24 21:37:25.000000000","message":"Interesting... Makes sense -- otherwise we\u0027d be doubled-up for lower partition numbers.\n\nAm I right to think that replica count is our only mechanism to trade off between balance and dispersion between the component rings? It seems like this restriction will make it a rather coarse-grained a tool for that... but accepting fractional replicas is probably better left as future work?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                \u0027All builders must have same value for %r.\\n%s\u0027"},{"line_number":58,"context_line":"                % (attr, \u0027\\n  \u0027.join(variations)))"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    # all ring builders should have int replica count and not have dirty mods"},{"line_number":61,"context_line":"    errors \u003d []"},{"line_number":62,"context_line":"    for index, builder in enumerate(builders):"},{"line_number":63,"context_line":"        if int(builder.replicas) !\u003d builder.replicas:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_1b09642b","line":60,"in_reply_to":"5ff73747_adb6c4d8","updated":"2017-04-25 08:45:31.000000000","message":"Supporting the fractional replica counts will be probably better rather than restriction but I think we need more deep consideration and perhaps we could need to take some tradeoff about the supporting this vs less rebalance because the change of assigned partition could cause huge rebalance stream.\n\nTo avoid the optimization debate, now we keep it as a restriction simply.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":77,"context_line":"    for builder in builders:"},{"line_number":78,"context_line":"        regions_info[builder] \u003d set("},{"line_number":79,"context_line":"            [dev[\u0027region\u0027] for dev in builder._iter_devs()])"},{"line_number":80,"context_line":"    for first, second in combinations(regions_info.items(), 2):"},{"line_number":81,"context_line":"        inter \u003d first[1] \u0026 second[1]"},{"line_number":82,"context_line":"        if inter:"},{"line_number":83,"context_line":"            raise ValueError(\u0027Same region found in different rings\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_ed2f2cf4","line":80,"range":{"start_line":80,"start_character":51,"end_line":80,"end_character":56},"updated":"2017-04-24 21:37:25.000000000","message":"Should we just use .values(), and drop the [1]s below?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":77,"context_line":"    for builder in builders:"},{"line_number":78,"context_line":"        regions_info[builder] \u003d set("},{"line_number":79,"context_line":"            [dev[\u0027region\u0027] for dev in builder._iter_devs()])"},{"line_number":80,"context_line":"    for first, second in combinations(regions_info.items(), 2):"},{"line_number":81,"context_line":"        inter \u003d first[1] \u0026 second[1]"},{"line_number":82,"context_line":"        if inter:"},{"line_number":83,"context_line":"            raise ValueError(\u0027Same region found in different rings\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_78faae42","line":80,"range":{"start_line":80,"start_character":51,"end_line":80,"end_character":56},"in_reply_to":"5ff73747_ed2f2cf4","updated":"2017-04-25 08:45:31.000000000","message":"I was not sure if the combinations works if the values are same each other (i.e. combinations([[1], [1], [1]], 2) could reduce the same combination like as only [([1], [1])] but it seems my mis-worried and the suggestion looks to work well.\n\nThanks.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            raise ValueError(\u0027Same region found in different rings\u0027)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"def check_for_dev_uniqueness(builders):"},{"line_number":87,"context_line":"    \"\"\""},{"line_number":88,"context_line":"    Check that no device appears in more than one of the given list of"},{"line_number":89,"context_line":"    builders."}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_6d5c3c4b","line":86,"updated":"2017-04-24 21:37:25.000000000","message":"Do we do any sort of comparable check for ordinary builders?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            raise ValueError(\u0027Same region found in different rings\u0027)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"def check_for_dev_uniqueness(builders):"},{"line_number":87,"context_line":"    \"\"\""},{"line_number":88,"context_line":"    Check that no device appears in more than one of the given list of"},{"line_number":89,"context_line":"    builders."}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_9b5454cc","line":86,"in_reply_to":"5ff73747_6d5c3c4b","updated":"2017-04-25 08:45:31.000000000","message":"Not sure I get your question correctly but currently it\u0027s NOT at the builder but at cli tool[1] so that it could be worth to backport some validator to the legacy RingBuilder class.\n1: https://github.com/openstack/swift/blob/master/swift/cli/ringbuilder.py#L653-L661","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        r2p2d \u003d copy.deepcopy(builder._replica2part2dev)"},{"line_number":129,"context_line":"        for part2dev in r2p2d:"},{"line_number":130,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":131,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":132,"context_line":"        for dev in [d for d in devs if d]:"},{"line_number":133,"context_line":"            # note that some devs may not be referenced in r2p2d but update"},{"line_number":134,"context_line":"            # their dev id nonetheless"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_60cc4389","line":131,"updated":"2017-04-24 21:37:25.000000000","message":"Do we have dev id compaction anywhere? Is that even feasible?\n\nI\u0027m a little worried that users may have built one giant ring (say, 4 regions with 5k devices each), then realize they want a composite ring, make a copy per region, prune them down to only the devices in that region, then zip them all together only to find that they\u0027ve exceeded the device limit. But maybe that just isn\u0027t something that would work, anyway -- is there a sane way to go from one Giant Ring of Doom to a composite ring?\n\nDo we ever handle the OverflowError if things go wrong?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        r2p2d \u003d copy.deepcopy(builder._replica2part2dev)"},{"line_number":129,"context_line":"        for part2dev in r2p2d:"},{"line_number":130,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":131,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":132,"context_line":"        for dev in [d for d in devs if d]:"},{"line_number":133,"context_line":"            # note that some devs may not be referenced in r2p2d but update"},{"line_number":134,"context_line":"            # their dev id nonetheless"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_aef58ccd","line":131,"in_reply_to":"5ff73747_60cc4389","updated":"2017-04-25 08:45:31.000000000","message":"Which part you are worried about the overflow? dev_id?\n\nAFAIK, an integer will be converted to long if it\u0027s over the integer range so systematically it doesn\u0027t seem a problem. Exactly compose giant rings will be a big ring instance but... is it same thing as we try to make a legacy ring instance? Am I missing something?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        r2p2d \u003d copy.deepcopy(builder._replica2part2dev)"},{"line_number":129,"context_line":"        for part2dev in r2p2d:"},{"line_number":130,"context_line":"            for part, dev in enumerate(part2dev):"},{"line_number":131,"context_line":"                part2dev[part] +\u003d device_offset"},{"line_number":132,"context_line":"        for dev in [d for d in devs if d]:"},{"line_number":133,"context_line":"            # note that some devs may not be referenced in r2p2d but update"},{"line_number":134,"context_line":"            # their dev id nonetheless"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_0dfeb4ad","line":131,"in_reply_to":"5ff73747_aef58ccd","updated":"2017-04-25 16:47:05.000000000","message":"@Tim are you thinking about component rings with lots of holes (i.e. None in dev list)?\n\nI did wonder whether we could have a non-linear mapping from component dev index to composite dev index, and eliminate all holes, but the linear mapping seemed simpler (particularly to debug a composite ring).","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":150,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":151,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":152,"context_line":"    \"\"\""},{"line_number":153,"context_line":"    pre_validate_all_builders(builders)"},{"line_number":154,"context_line":"    check_for_dev_uniqueness(builders)"},{"line_number":155,"context_line":"    rd \u003d _make_composite_ring(builders)"},{"line_number":156,"context_line":"    return rd"},{"line_number":157,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_833bd9cf","line":154,"range":{"start_line":153,"start_character":4,"end_line":154,"end_character":38},"updated":"2017-04-24 21:37:25.000000000","message":"Any reason not to just roll these two together?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":150,"context_line":"    :return: a new RingData instance built from the component builders"},{"line_number":151,"context_line":"    :raises ValueError: if the builders are invalid with respect to each other"},{"line_number":152,"context_line":"    \"\"\""},{"line_number":153,"context_line":"    pre_validate_all_builders(builders)"},{"line_number":154,"context_line":"    check_for_dev_uniqueness(builders)"},{"line_number":155,"context_line":"    rd \u003d _make_composite_ring(builders)"},{"line_number":156,"context_line":"    return rd"},{"line_number":157,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_0eab6089","line":154,"range":{"start_line":153,"start_character":4,"end_line":154,"end_character":38},"in_reply_to":"5ff73747_833bd9cf","updated":"2017-04-25 08:45:31.000000000","message":"Ah, just from a historical reason for the past patch sets so that, now we can merge them (or call check_for_dev_uniquness in pre_validate_all_builders), IMO.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":166,"context_line":"def _make_composite_metadata(builders):"},{"line_number":167,"context_line":"    # return a list of component builder metadata"},{"line_number":168,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":169,"context_line":"    return {\u0027version\u0027: 1, \u0027components\u0027: component_meta}"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_be870cd8","line":169,"range":{"start_line":169,"start_character":23,"end_line":169,"end_character":24},"updated":"2017-04-24 21:37:25.000000000","message":"Am I right to think that composite rings are *only ever* at version 1? I don\u0027t see us incrementing this anywhere...","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":166,"context_line":"def _make_composite_metadata(builders):"},{"line_number":167,"context_line":"    # return a list of component builder metadata"},{"line_number":168,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":169,"context_line":"    return {\u0027version\u0027: 1, \u0027components\u0027: component_meta}"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_0dec74f1","line":169,"range":{"start_line":169,"start_character":23,"end_line":169,"end_character":24},"in_reply_to":"5ff73747_0e99206d","updated":"2017-04-25 16:47:05.000000000","message":"This was not intended to be \u0027version\u0027 of the composite ring, incremented when it changes. It was intended to be a version for the data structure typing, added at John\u0027s request on etherpad.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":166,"context_line":"def _make_composite_metadata(builders):"},{"line_number":167,"context_line":"    # return a list of component builder metadata"},{"line_number":168,"context_line":"    component_meta \u003d [_make_component_meta(builder) for builder in builders]"},{"line_number":169,"context_line":"    return {\u0027version\u0027: 1, \u0027components\u0027: component_meta}"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"def check_same_builder(old_component, new_component):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_0e99206d","line":169,"range":{"start_line":169,"start_character":23,"end_line":169,"end_character":24},"in_reply_to":"5ff73747_be870cd8","updated":"2017-04-25 08:45:31.000000000","message":"True, currently.\n\nThe version will be incremented by CompositeRingBuilder, IIUC.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    :raises ValueError: if the new_component is not the same as that described"},{"line_number":183,"context_line":"    by the old_component"},{"line_number":184,"context_line":"    \"\"\""},{"line_number":185,"context_line":"    for key in [\u0027replicas\u0027, \u0027id\u0027]:"},{"line_number":186,"context_line":"        if old_component[key] !\u003d new_component[key]:"},{"line_number":187,"context_line":"            raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":188,"context_line":"                             (key, old_component[key], new_component[key]))"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_43acb18b","line":185,"updated":"2017-04-24 21:37:25.000000000","message":"So replicas, once set, can\u0027t change?\n\nI was thinking that if you already had one DC up and had a second that was coming up but not fully online, you might want to use replicas to weight your 2x (7 + 4) as 16 / 6 or something and slowly move toward 11 / 11 as hardware became available... but I guess that won\u0027t work.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    :raises ValueError: if the new_component is not the same as that described"},{"line_number":183,"context_line":"    by the old_component"},{"line_number":184,"context_line":"    \"\"\""},{"line_number":185,"context_line":"    for key in [\u0027replicas\u0027, \u0027id\u0027]:"},{"line_number":186,"context_line":"        if old_component[key] !\u003d new_component[key]:"},{"line_number":187,"context_line":"            raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":188,"context_line":"                             (key, old_component[key], new_component[key]))"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_e8d2565c","line":185,"in_reply_to":"5ff73747_0e5c807c","updated":"2017-04-25 16:47:05.000000000","message":"See Clay\u0027s comment on patchset 11, April 4th.\n\nWith an EC policy if a composite ring changed from having (N replicas, M replicas) to (N+1 replicas, M-1 replicas) then M fragments of every object would become mislocated because the node indexes would be shuffled.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    :raises ValueError: if the new_component is not the same as that described"},{"line_number":183,"context_line":"    by the old_component"},{"line_number":184,"context_line":"    \"\"\""},{"line_number":185,"context_line":"    for key in [\u0027replicas\u0027, \u0027id\u0027]:"},{"line_number":186,"context_line":"        if old_component[key] !\u003d new_component[key]:"},{"line_number":187,"context_line":"            raise ValueError(\"Attribute mismatch for %s: %r !\u003d %r\" %"},{"line_number":188,"context_line":"                             (key, old_component[key], new_component[key]))"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_0e5c807c","line":185,"in_reply_to":"5ff73747_43acb18b","updated":"2017-04-25 08:45:31.000000000","message":"Correct. This function is designed as you think.\n\nHowever, one point you may be missing is that this function is for continuous rebalance operation of the component rings. This function will be called only in check_against_exisiting function and if the older ring not exists, the check will be skipped so currently we assume the replica change should not be in regular ring operation but should do carefully by operator, i think.\n\nSo now, *maiking it as a new ring* is only a way to create another id, replica count for the component ring.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    Check that the given list of builders is the same as that used to build an"},{"line_number":212,"context_line":"    existing composite ring. Return True if any of the given builders has been"},{"line_number":213,"context_line":"    modified with respect to its state when the given component_meta was"},{"line_number":214,"context_line":"    created."},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"    :param old_composite_meta: a list of dicts of metadata, each dict"},{"line_number":217,"context_line":"    describing a component ring of an existing composite ring"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_63520d6a","line":214,"updated":"2017-04-24 21:37:25.000000000","message":"Maybe worth calling out that the components have to be in the same order?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    Check that the given list of builders is the same as that used to build an"},{"line_number":212,"context_line":"    existing composite ring. Return True if any of the given builders has been"},{"line_number":213,"context_line":"    modified with respect to its state when the given component_meta was"},{"line_number":214,"context_line":"    created."},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"    :param old_composite_meta: a list of dicts of metadata, each dict"},{"line_number":217,"context_line":"    describing a component ring of an existing composite ring"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_ee7154e2","line":214,"in_reply_to":"5ff73747_63520d6a","updated":"2017-04-25 08:45:31.000000000","message":"Good call, thanks.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    for builder_id, index in id2index.items():"},{"line_number":268,"context_line":"        if len(index) \u003e 1:"},{"line_number":269,"context_line":"            raise ValueError(\"Builder id %r used at indexes %s\" %"},{"line_number":270,"context_line":"                             (builder_id, \u0027, \u0027.join(index)))"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"def write_composite_ring(builders, composite_ring_file):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_f91ece6e","line":270,"updated":"2017-04-24 21:37:25.000000000","message":"Seems kinda weird to mix the two styles of error reporting... maybe do something like\n\n    for builder_id, index in id2index.items():\n        if len(index) \u003e 1:\n            errors.append(\"Builder id %r used at indexes %s\" %\n                          (builder_id, \u0027, \u0027.join(index)))\n    if errors:\n        raise ValueError(\u0027\\n\u0027.join(errors))\n\ninstead?","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    for builder_id, index in id2index.items():"},{"line_number":268,"context_line":"        if len(index) \u003e 1:"},{"line_number":269,"context_line":"            raise ValueError(\"Builder id %r used at indexes %s\" %"},{"line_number":270,"context_line":"                             (builder_id, \u0027, \u0027.join(index)))"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"def write_composite_ring(builders, composite_ring_file):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_b10f312d","line":270,"in_reply_to":"5ff73747_f91ece6e","updated":"2017-04-25 08:45:31.000000000","message":"Done","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e14a80f4138ca1718cc500db26d704311b6d0ed9","unresolved":false,"context_lines":[{"line_number":270,"context_line":"                             (builder_id, \u0027, \u0027.join(index)))"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"def write_composite_ring(builders, composite_ring_file):"},{"line_number":274,"context_line":"    \"\"\""},{"line_number":275,"context_line":"    Given a list of component ring builders, build a composite ring and"},{"line_number":276,"context_line":"    write it to the given composite ring file, along with composite ring"}],"source_content_type":"text/x-python","patch_set":16,"id":"7ffa3b31_ed0b2a27","line":273,"updated":"2017-04-20 16:38:00.000000000","message":"I\u0027m not sure if we want this function or a class such as prototyped in patch [1]\n\n[1] https://review.openstack.org/#/c/453827/3","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":270,"context_line":"                             (builder_id, \u0027, \u0027.join(index)))"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"def write_composite_ring(builders, composite_ring_file):"},{"line_number":274,"context_line":"    \"\"\""},{"line_number":275,"context_line":"    Given a list of component ring builders, build a composite ring and"},{"line_number":276,"context_line":"    write it to the given composite ring file, along with composite ring"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_d13bddce","line":273,"in_reply_to":"7ffa3b31_ed0b2a27","updated":"2017-04-25 08:45:31.000000000","message":"I think class would be good. I\u0027ll check the patch too later.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":290,"context_line":"    if os.path.exists(composite_ring_file):"},{"line_number":291,"context_line":"        # to avoid inadvertently mangling an existing composite ring, load it"},{"line_number":292,"context_line":"        # and check that the current list of builders is consistent with those"},{"line_number":293,"context_line":"        # use to compose the existing ring"},{"line_number":294,"context_line":"        rd \u003d RingData.load(composite_ring_file, metadata_only\u003dTrue)"},{"line_number":295,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":296,"context_line":"        if old_composite_meta is None:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_9e121058","line":293,"range":{"start_line":293,"start_character":10,"end_line":293,"end_character":13},"updated":"2017-04-24 21:37:25.000000000","message":"nit: s/use/used/","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":290,"context_line":"    if os.path.exists(composite_ring_file):"},{"line_number":291,"context_line":"        # to avoid inadvertently mangling an existing composite ring, load it"},{"line_number":292,"context_line":"        # and check that the current list of builders is consistent with those"},{"line_number":293,"context_line":"        # use to compose the existing ring"},{"line_number":294,"context_line":"        rd \u003d RingData.load(composite_ring_file, metadata_only\u003dTrue)"},{"line_number":295,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":296,"context_line":"        if old_composite_meta is None:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_31520115","line":293,"range":{"start_line":293,"start_character":10,"end_line":293,"end_character":13},"in_reply_to":"5ff73747_9e121058","updated":"2017-04-25 08:45:31.000000000","message":"Done","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":296,"context_line":"        if old_composite_meta is None:"},{"line_number":297,"context_line":"            raise ValueError(\"Not a composite ring file: %s\" %"},{"line_number":298,"context_line":"                             composite_ring_file)"},{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_9e40d037","line":298,"updated":"2017-04-24 21:37:25.000000000","message":"Good call! Do we have any comparable checks when we try to perform operations on what we expect to be ordinary rings?\n\nLike, would it be bad to create a builder directly from a composite ring? My gut says that\u0027s not a good idea.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":296,"context_line":"        if old_composite_meta is None:"},{"line_number":297,"context_line":"            raise ValueError(\"Not a composite ring file: %s\" %"},{"line_number":298,"context_line":"                             composite_ring_file)"},{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_a828ce33","line":298,"in_reply_to":"5ff73747_9e40d037","updated":"2017-04-25 16:47:05.000000000","message":"Interesting. So we still have an open question as to whether we even write composite metadata to a ring.gz or only write it to a composite-ring-json file. Is this an argument for including composite meta in ring.gz?\n\nThat said, IIUC people do not generally work with builder files not ring.gz, so would not be loading a composite ring.gz anyway.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d6effeb84c52a685fa72bb4f36b9250b41b16334","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        old_composite_meta \u003d rd.metadata.get(\u0027composite_info\u0027)"},{"line_number":296,"context_line":"        if old_composite_meta is None:"},{"line_number":297,"context_line":"            raise ValueError(\"Not a composite ring file: %s\" %"},{"line_number":298,"context_line":"                             composite_ring_file)"},{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_f6c0eb7e","line":298,"in_reply_to":"5ff73747_a828ce33","updated":"2017-04-26 02:32:51.000000000","message":"I\u0027ve been creating builders from composite/zipped rings - it\u0027s one of the benefits of \"it\u0027s just a ring\" - I can run the resulting builder through the dispersion command and get the lay of the land.\n\nIf we end up with with an on-disk file to present a CompositeRingBuilder I think avoiding calling it just `.builder` (maybe `.composite`?) would avoid a `.builder` from a dumped composite.ring.gz getting mixed up over-writing a `object-1.composite`","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"},{"line_number":302,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_7e94e48b","line":302,"range":{"start_line":302,"start_character":18,"end_line":302,"end_character":28},"updated":"2017-04-24 21:37:25.000000000","message":"Maybe worth creating a new exception type? This seems like a much less severe sort of an error than any of the other ValueErrors this module may raise.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"},{"line_number":302,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_089a226d","line":302,"range":{"start_line":302,"start_character":18,"end_line":302,"end_character":28},"in_reply_to":"5ff73747_11f3850c","updated":"2017-04-25 16:47:05.000000000","message":"here [1] I provide a force flag to bypass all checks but that does not discriminate between the severity of them.\n\n[1] https://review.openstack.org/#/c/453827/3/swift/common/ring/composite_builder.py@377","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":299,"context_line":"        modified \u003d check_against_existing(old_composite_meta,"},{"line_number":300,"context_line":"                                          new_composite_meta)"},{"line_number":301,"context_line":"        if not modified:"},{"line_number":302,"context_line":"            raise ValueError(\"None of the component builders has been modified\""},{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_11f3850c","line":302,"range":{"start_line":302,"start_character":18,"end_line":302,"end_character":28},"in_reply_to":"5ff73747_7e94e48b","updated":"2017-04-25 08:45:31.000000000","message":"perhaps, swift.common.exception.RingBuilderError? or sort of newly-created CompositeRingBuilderError.\n\nPerhaps, it could be focused at the Alistair\u0027s composite ring builder patch.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e14a80f4138ca1718cc500db26d704311b6d0ed9","unresolved":false,"context_lines":[{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":306,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"},{"line_number":307,"context_line":"    rd.save(composite_ring_file)"}],"source_content_type":"text/x-python","patch_set":16,"id":"7ffa3b31_08140443","line":306,"updated":"2017-04-20 16:38:00.000000000","message":"on reflection I think the metadata should be set when the RingData is created, see follow on patch [1]\n\n[1] https://review.openstack.org/#/c/458585/1","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"48797fa409dc15c889207cecef4141bcdc7b4ca1","unresolved":false,"context_lines":[{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":306,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"},{"line_number":307,"context_line":"    rd.save(composite_ring_file)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_28e5dea8","line":306,"in_reply_to":"5ff73747_31d7a14e","updated":"2017-04-25 16:47:05.000000000","message":"@Tim yep, see https://review.openstack.org/#/c/453827","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"c5bb33ae8aca8a0f93c68b7119e673278391bd93","unresolved":false,"context_lines":[{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":306,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"},{"line_number":307,"context_line":"    rd.save(composite_ring_file)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_31d7a14e","line":306,"in_reply_to":"5ff73747_de3ad8e5","updated":"2017-04-25 08:45:31.000000000","message":"I need to look at the follow up at first, just ack as I noticed we need to discuss on that.","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b09a720ce9a18d930ff8a398ce8438af074dcdec","unresolved":false,"context_lines":[{"line_number":303,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":306,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"},{"line_number":307,"context_line":"    rd.save(composite_ring_file)"}],"source_content_type":"text/x-python","patch_set":16,"id":"5ff73747_de3ad8e5","line":306,"in_reply_to":"7ffa3b31_08140443","updated":"2017-04-24 21:37:25.000000000","message":"So the only place we ever record the components is in the ring itself? After reading Clay\u0027s comments about the metadata, I\u0027m a little nervous about using something rather ephemeral as the only source of truth...","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":64,"context_line":"            errors.append("},{"line_number":65,"context_line":"                \u0027Non integer replica count %s found at index %s\u0027 %"},{"line_number":66,"context_line":"                (builder.replicas, index))"},{"line_number":67,"context_line":"        if builder.devs_changed:"},{"line_number":68,"context_line":"            errors.append("},{"line_number":69,"context_line":"                \u0027Builder needs rebalance to apply changes at index %s\u0027 %"},{"line_number":70,"context_line":"                index)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_d180598e","line":67,"updated":"2017-04-26 03:37:58.000000000","message":"Yay! +1000","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":288,"context_line":"    :raises: ValueError if the component ring builders are not suitable for"},{"line_number":289,"context_line":"        composing with each other, or are inconsistent with any existing"},{"line_number":290,"context_line":"        composite ring, or if there has been no change with respect to the"},{"line_number":291,"context_line":"        existing ring."},{"line_number":292,"context_line":"    \"\"\""},{"line_number":293,"context_line":"    check_builder_ids(builders)"},{"line_number":294,"context_line":"    new_composite_meta \u003d _make_composite_metadata(builders)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_d6fdef3a","line":291,"updated":"2017-04-26 03:37:58.000000000","message":"this method is not great for me; I like the class [1] a lot better.\n\n1. https://review.openstack.org/#/c/453827","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1d1818e38889d826b5d261ac631dc1e597eb791e","unresolved":false,"context_lines":[{"line_number":288,"context_line":"    :raises: ValueError if the component ring builders are not suitable for"},{"line_number":289,"context_line":"        composing with each other, or are inconsistent with any existing"},{"line_number":290,"context_line":"        composite ring, or if there has been no change with respect to the"},{"line_number":291,"context_line":"        existing ring."},{"line_number":292,"context_line":"    \"\"\""},{"line_number":293,"context_line":"    check_builder_ids(builders)"},{"line_number":294,"context_line":"    new_composite_meta \u003d _make_composite_metadata(builders)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_30e2bf30","line":291,"in_reply_to":"5ff73747_d6fdef3a","updated":"2017-04-26 17:00:28.000000000","message":"I\u0027m going to remove this function in the dependent patch [1] and then if we like it we can squash to here\n\n[1] https://review.openstack.org/#/c/453827","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":292,"context_line":"    \"\"\""},{"line_number":293,"context_line":"    check_builder_ids(builders)"},{"line_number":294,"context_line":"    new_composite_meta \u003d _make_composite_metadata(builders)"},{"line_number":295,"context_line":"    if os.path.exists(composite_ring_file):"},{"line_number":296,"context_line":"        # to avoid inadvertently mangling an existing composite ring, load it"},{"line_number":297,"context_line":"        # and check that the current list of builders is consistent with those"},{"line_number":298,"context_line":"        # used to compose the existing ring"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_f6834ba8","line":295,"updated":"2017-04-26 03:37:58.000000000","message":"this is the only reason I can use this change - when the ring file doesn\u0027t exist at the path I want to write it it\u0027s like I\u0027m creating a new CompositeBuilder every time\n\nIn fact... I think at this point I should just be careful and and use compose_rings","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"aae8cd98a8f51b0d5d544d2efd8c15efd63fbdae","unresolved":false,"context_lines":[{"line_number":308,"context_line":"                             \" since the existing composite ring was built.\")"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"    rd \u003d compose_rings(builders)"},{"line_number":311,"context_line":"    rd.metadata.update({\u0027composite_info\u0027: new_composite_meta})"},{"line_number":312,"context_line":"    rd.save(composite_ring_file)"}],"source_content_type":"text/x-python","patch_set":18,"id":"5ff73747_e5752f65","line":311,"updated":"2017-05-02 10:42:59.000000000","message":"hmmm, CompositeRingBuilder does not yet install metadata in the ring data - but we have support for it in RingData.\n\nIMHO *IF* we are going to do this then the right approach is in this dependent patch https://review.openstack.org/#/c/458585","commit_id":"09245e10e97962d9fd7c2b36ef20f937e9bb2891"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4d5b7616272d90c3c2947bcbe5e02bfa2411f1b0","unresolved":false,"context_lines":[{"line_number":14,"context_line":"# limitations under the License."},{"line_number":15,"context_line":"\"\"\""},{"line_number":16,"context_line":"A standard ring built using the :ref:`ring-builder \u003cring_builder\u003e` will attempt"},{"line_number":17,"context_line":"to randomly disperse replicas or erasure-coded fragments across failure"},{"line_number":18,"context_line":"domains, but does not provide any guarantees such as placing at least one"},{"line_number":19,"context_line":"replica of every partition into each region. Composite rings are intended to"},{"line_number":20,"context_line":"provide operators with greater control over the dispersion of object replicas"}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_66cc3b6a","line":17,"range":{"start_line":17,"start_character":33,"end_line":17,"end_character":56},"updated":"2017-05-16 22:02:40.000000000","message":"For follow-up: This is the first time we ever discuss erasure coding on doc/overview_ring.html -- I wonder if we should have *some* sort of discussion about it when we first introduce replica count?","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4d5b7616272d90c3c2947bcbe5e02bfa2411f1b0","unresolved":false,"context_lines":[{"line_number":20,"context_line":"provide operators with greater control over the dispersion of object replicas"},{"line_number":21,"context_line":"or fragments across a cluster, in particular when there is a desire to"},{"line_number":22,"context_line":"guarantee that some replicas or fragments are placed in certain failure"},{"line_number":23,"context_line":"domains."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"A composite ring comprises two or more component rings that are combined to"},{"line_number":26,"context_line":"form a single ring with a replica count equal to the sum of the component"}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_2622b301","line":23,"updated":"2017-05-16 22:02:40.000000000","message":"This sounds very similar to overload -- is it worth calling out that composite rings provide strict guarantees, while overload offers more, IDK, probabilistic help?\n\nIt also seems like we ought to call out duplicated EC fragments and their per-region issues, as that was (in my mind, anyway) the primary motivator for this.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":20,"context_line":"provide operators with greater control over the dispersion of object replicas"},{"line_number":21,"context_line":"or fragments across a cluster, in particular when there is a desire to"},{"line_number":22,"context_line":"guarantee that some replicas or fragments are placed in certain failure"},{"line_number":23,"context_line":"domains."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"A composite ring comprises two or more component rings that are combined to"},{"line_number":26,"context_line":"form a single ring with a replica count equal to the sum of the component"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_b964e2ba","line":23,"in_reply_to":"1f013ff3_2622b301","updated":"2017-05-18 08:56:51.000000000","message":"Those are also in follow-up.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4d5b7616272d90c3c2947bcbe5e02bfa2411f1b0","unresolved":false,"context_lines":[{"line_number":78,"context_line":"    The ``id`` of each component RingBuilder is therefore stored in metadata of"},{"line_number":79,"context_line":"    the composite and used to check for the component ordering when the same"},{"line_number":80,"context_line":"    composite ring is re-composed. RingBuilder id\u0027s are only assigned when a"},{"line_number":81,"context_line":"    RingBuilder instance is first saved. Older RingBuilders instances loaded"},{"line_number":82,"context_line":"    from file may not have an ``id`` assigned and will need to be saved before"},{"line_number":83,"context_line":"    they can be used as components of a composite ring. This can be achieved"},{"line_number":84,"context_line":"    by, for example::"}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_c1cad918","line":81,"range":{"start_line":81,"start_character":47,"end_line":81,"end_character":69},"updated":"2017-05-16 22:02:40.000000000","message":"nit: \"RingBuilder instances\" or \"RingBuilders\"","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":78,"context_line":"    The ``id`` of each component RingBuilder is therefore stored in metadata of"},{"line_number":79,"context_line":"    the composite and used to check for the component ordering when the same"},{"line_number":80,"context_line":"    composite ring is re-composed. RingBuilder id\u0027s are only assigned when a"},{"line_number":81,"context_line":"    RingBuilder instance is first saved. Older RingBuilders instances loaded"},{"line_number":82,"context_line":"    from file may not have an ``id`` assigned and will need to be saved before"},{"line_number":83,"context_line":"    they can be used as components of a composite ring. This can be achieved"},{"line_number":84,"context_line":"    by, for example::"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_b9d6224f","line":81,"range":{"start_line":81,"start_character":47,"end_line":81,"end_character":69},"in_reply_to":"1f013ff3_c1cad918","updated":"2017-05-18 08:56:51.000000000","message":"That is addressed at https://review.openstack.org/#/c/465184/","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c475efbc2a9a6ae47cd3d37bfdc97f3dae7a307e","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    they can be used as components of a composite ring. This can be achieved"},{"line_number":84,"context_line":"    by, for example::"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"        swift-ring-builder \u003cbuilder-file\u003e rebalance --force"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"\"\"\""},{"line_number":89,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_f1c5c28f","line":86,"updated":"2017-05-16 00:18:28.000000000","message":"I thought Christian said this was a bad way to re-write a .ring.gz!?","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4f6c669513cb2a11c69c8dbd1379af53131030c6","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    they can be used as components of a composite ring. This can be achieved"},{"line_number":84,"context_line":"    by, for example::"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"        swift-ring-builder \u003cbuilder-file\u003e rebalance --force"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"\"\"\""},{"line_number":89,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_3268b11f","line":86,"in_reply_to":"1f013ff3_f1c5c28f","updated":"2017-05-16 17:28:20.000000000","message":"We don\u0027t need to rewrite the .ring.gz, but rather the .builder. However, the obvious thing to do it is apparently cowardly:\n\n $ swift-ring-builder x create 4 3 0\n $ swift-ring-builder x write_builder\n Cowardly refusing to overwrite existing Ring Builder file: x\n\nI guess the better way is to re-set the min_part_hours?\n\n swift-ring-builder x set_min_part_hours \u003cexisting min_part_hours\u003e","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4d5b7616272d90c3c2947bcbe5e02bfa2411f1b0","unresolved":false,"context_lines":[{"line_number":147,"context_line":"    regions_info \u003d {}"},{"line_number":148,"context_line":"    for builder in builders:"},{"line_number":149,"context_line":"        regions_info[builder] \u003d set("},{"line_number":150,"context_line":"            [dev[\u0027region\u0027] for dev in builder._iter_devs()])"},{"line_number":151,"context_line":"    for first_region_set, second_region_set in combinations("},{"line_number":152,"context_line":"            regions_info.values(), 2):"},{"line_number":153,"context_line":"        inter \u003d first_region_set \u0026 second_region_set"}],"source_content_type":"text/x-python","patch_set":21,"id":"1f013ff3_46093f2f","line":150,"range":{"start_line":150,"start_character":12,"end_line":150,"end_character":13},"updated":"2017-05-16 22:02:40.000000000","message":"nit: we can just use the generator here; no need for the intermediary list.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":147,"context_line":"    regions_info \u003d {}"},{"line_number":148,"context_line":"    for builder in builders:"},{"line_number":149,"context_line":"        regions_info[builder] \u003d set("},{"line_number":150,"context_line":"            [dev[\u0027region\u0027] for dev in builder._iter_devs()])"},{"line_number":151,"context_line":"    for first_region_set, second_region_set in combinations("},{"line_number":152,"context_line":"            regions_info.values(), 2):"},{"line_number":153,"context_line":"        inter \u003d first_region_set \u0026 second_region_set"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_79b64adc","line":150,"range":{"start_line":150,"start_character":12,"end_line":150,"end_character":13},"in_reply_to":"1f013ff3_46093f2f","updated":"2017-05-18 08:56:51.000000000","message":"in follow-up too.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":372,"context_line":"        ring_data \u003d crb.compose()"},{"line_number":373,"context_line":""},{"line_number":374,"context_line":"        # save the composite ring file"},{"line_number":375,"context_line":"        ring_data.save(\"composite_ring.gz\"\")"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"        # save the composite metadata file"},{"line_number":378,"context_line":"        crb.save(\"composite_builder.composite\")"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_16283884","line":375,"range":{"start_line":375,"start_character":42,"end_line":375,"end_character":43},"updated":"2017-05-18 08:56:51.000000000","message":"nits) redundant double quote.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"}],"swift/common/ring/ring.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ff4781fe7468c7cf1d23a0c033e52dbb0823c9bc","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_be223451","line":45,"updated":"2017-04-04 03:30:00.000000000","message":"I don\u0027t know if leaving this completely unstructured is ideal.  In the past when we\u0027ve wanted to annotate stuff for internal reasons we have to worry about name conflicts and the like because we don\u0027t know how people use it.\n\nFurther I\u0027m not 100% we want to open the door for people to keep metadata with the *rings* (as opposed to the builder).  The ring objects are supposed to be ephemeral - the artifact that is the result of a change persisted in the builder after a rebalance.  If we add metadata to the rings objects it needs to be as epheneral as the rings them selves - we shouldn\u0027t expect the dumped ring data to be persisted and backed up to the same degree we expect .builder files to be managed.\n\nEssentially .builders are gold - you can write rings to /tmp because everything you might need/want from a ring comes out of .builder(s)","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"9c6fa2a79342af3a6367efeedd260f58b2b10eac","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_cf7e526a","line":45,"in_reply_to":"dfeb2761_be223451","updated":"2017-04-04 08:53:13.000000000","message":"Ah, I think the idea is correct, we should manage the persistent .builder file and the ring file is just a dump of that.\n\nOn the other hand, I think it\u0027s worth if we can dump the *builder* metadata into the ring which enables to trace by the ring to the builder.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"647ed684a8ab149105124bec9f0b9d2708343cf4","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_d6870b61","line":45,"in_reply_to":"dfeb2761_cf7e526a","updated":"2017-04-04 11:20:04.000000000","message":"I am learning more about how ring.gz\u0027s are perceived vs builder files, and their different lifeycles, so thanks for the comments.\n\nWith my increasing undertanding, I do agree that having \u0027gold\u0027 file with the composite metadata is a good thing. But we didn\u0027t yet reach a conclusion on what those files would be: put it in a .builder file and we need to make swift-ring-builder aware of these other types of builder files, or at least convince ourselves that it\u0027s harmless; or put it in a different file extension.\n\nIn the meantime, is there harm in having the metadata *also* be in the ring file? It does at least mean that the ring.gz has sufficient information to know what it is (so a .builder or .json or whatever could be recovered from a ring.gz). Unlike a normal ring.gz, you really cannot recover a composite builder with something \u0027swift-ring-builder \u003cring.gz\u003e write_builder\u0027 unless there is some composite metadata written in the composite ring.gz.\n\nSo do we say \u0027if you lose a normal builder file, you can recover, but if you lose a composite-builder file*, you have no chance, unless you wrote down the composition state somewhere else\u0027?\n\n(* yet to be implemented of course)\n\nTo be clear, I *do* see the value in a composite-builder like file separate to the ring.gz, but I also see value in embedding composite metadata in the ring.gz file *as well*.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"63de9b959ee15b1ad8f9d616a74a65e6d292bd00","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_536ac8ec","line":45,"in_reply_to":"dfeb2761_d6870b61","updated":"2017-04-05 07:27:05.000000000","message":"+1","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0a4e94e303c2833c0df91170ddcdd2189a161dfb","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfeb2761_a593185b","line":45,"in_reply_to":"dfeb2761_d6870b61","updated":"2017-04-05 19:45:42.000000000","message":"Yes, \"also as well\" - the consideration of the yet to be implemented \"write_builder*s* from composite .ring.gz\" is convincing.\n\nHowever, there\u0027s some memory concerns (e.g. we don\u0027t put min_part_hours array to make .ring\u0027s smaller than .builders) - but I think that\u0027s nominal here (nevertheless we *do* skip a number of other attributes would be \"cheap\" compared to replica2part2dev for no good reason other than \"they\u0027re not needed\").  Along those lines I *might* argue we *could* drop validation related information and *only* store what we need to do the job at hand (i.e. replicas_per_ring \u003d [3, 3] ... and filter the dev list based on the id\u0027s in the table?)\n\nThe invention of write_builder was sort of a \"why not\" - I\u0027m not 100% I\u0027ve ever met someone who actually lost their builders (i occasionally dump a ring to a builder on a node out of convenience so I can inspect dispersion with swift-ring-builder or w/e) - I\u0027m sort of mad/bummed that supporting the use-case causes us grief now.  You need to keep backups of your builders.\n\nAlternatively, I could imagine write_builder growing an slice syntax too... assuming you remember how your component rings were laid out you could call \"write_builder 0-2 region1.builder\" and \"write_builder 3-5 region2.builder\"???\n\nAt a minimum if we add generic \"metadata\" like this we should use namespaces - otherwise let\u0027s just make an attribute composite_metadata.","commit_id":"3068942e5dd948fca502f643851be68f58bf6d44"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e14a80f4138ca1718cc500db26d704311b6d0ed9","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":16,"id":"7ffa3b31_6d203aad","line":45,"updated":"2017-04-20 16:38:00.000000000","message":"there is suggestion in previous reviews that this unstructured metadata is not desirable - see this follow on [1] which changes to a class attribute\n\n[1] https://review.openstack.org/#/c/458585/1","commit_id":"88c7f2da86bc3513b972540fe2b581935d19a331"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_c543f7ae","line":45,"updated":"2017-04-26 03:37:58.000000000","message":"it\u0027s not obvious why this isn\u0027t an orthogonal change to composite buildering\n\noh right, it\u0027s because we use the .ring.gz to persist the composite_meta between zips!  It\u0027s not stored anywhere else.  Hang on to your rings people!","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9866b56e3df75e45b78fc00b38be2711f203b247","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.devs \u003d devs"},{"line_number":43,"context_line":"        self._replica2part2dev_id \u003d replica2part2dev_id"},{"line_number":44,"context_line":"        self._part_shift \u003d part_shift"},{"line_number":45,"context_line":"        self.metadata \u003d {} if metadata is None else metadata"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        for dev in self.devs:"},{"line_number":48,"context_line":"            if dev is not None:"}],"source_content_type":"text/x-python","patch_set":19,"id":"5ff73747_e5facfba","line":45,"updated":"2017-05-02 10:24:47.000000000","message":"we\u0027re not yet using this in CompositeRingBuilder\n\nIF we are going to use it then I prefer the approach in https://review.openstack.org/#/c/458585","commit_id":"183a867b243046e6c451350e75e7885211b0c041"}],"test/unit/cli/test_default_output_id_assigned.stub":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":1,"context_line":"__RINGFILE__, build version 4, id fake_builder_id"},{"line_number":2,"context_line":"64 partitions, 3.000000 replicas, 4 regions, 4 zones, 4 devices, 100.00 balance, 0.00 dispersion"},{"line_number":3,"context_line":"The minimum number of hours before a partition can be reassigned is 1 (0:00:00 remaining)"},{"line_number":4,"context_line":"The overload factor is 0.00% (0.000000)"}],"source_content_type":"application/octet-stream","patch_set":17,"id":"5ff73747_e591539b","line":1,"updated":"2017-04-26 03:37:58.000000000","message":"oic, fake_builder_id just forces all test rings to get the same instead of a template substitution solution like __RINGFILE__","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"}],"test/unit/cli/test_ipv6_output.stub":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":1,"context_line":"__RINGFILE__, build version 4, id fake_builder_id"},{"line_number":2,"context_line":"256 partitions, 3.000000 replicas, 4 regions, 4 zones, 4 devices, 100.00 balance, 0.00 dispersion"},{"line_number":3,"context_line":"The minimum number of hours before a partition can be reassigned is 1 (0:00:00 remaining)"},{"line_number":4,"context_line":"The overload factor is 0.00% (0.000000)"}],"source_content_type":"application/octet-stream","patch_set":17,"id":"5ff73747_6528a333","line":1,"updated":"2017-04-26 03:37:58.000000000","message":"the idea with __RINGFILE__ is that assertOutputStub stub would have the *option* of saying \"i don\u0027t care what the tempfile is\"\n\nI\u0027m not sure if we need/want to assertOutputStub to support something similar for __BUILDERID__","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"}],"test/unit/cli/test_ringbuilder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":112,"context_line":"                self.fail(\u0027%r does not exist\u0027 % filepath)"},{"line_number":113,"context_line":"            else:"},{"line_number":114,"context_line":"                self.fail(\u0027%r could not be read (%s)\u0027 % (filepath, e))"},{"line_number":115,"context_line":"        output \u003d output.replace(self.tempfile, \u0027__RINGFILE__\u0027)"},{"line_number":116,"context_line":"        for i, (value, expected) in enumerate("},{"line_number":117,"context_line":"                itertools.izip_longest("},{"line_number":118,"context_line":"                    output.splitlines(), stub.splitlines())):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_05aaffd0","line":115,"updated":"2017-04-26 03:37:58.000000000","message":"are these attributes different enough to warrant different solutions?\n\n    output.replace(RingBuilder.load(self.tempfile)._id, \u0027__BUILDERID__\u0027)\n\n/me shrugs","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":180,"context_line":"                      })"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        with mock.patch(\u0027uuid.uuid4\u0027, return_value\u003dmock.Mock(hex\u003dbuilder_id)):"},{"line_number":183,"context_line":"            ring.save(self.tmpfile)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"    def assertSystemExit(self, return_code, func, *argv):"},{"line_number":186,"context_line":"        with self.assertRaises(SystemExit) as cm:"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_455a472a","line":183,"updated":"2017-04-26 03:37:58.000000000","message":"so this is where the stub\u0027s fake_builder_id get\u0027s fixed","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":1704,"context_line":"                      })"},{"line_number":1705,"context_line":""},{"line_number":1706,"context_line":"        with mock.patch("},{"line_number":1707,"context_line":"                \u0027uuid.uuid4\u0027, return_value\u003dmock.Mock(hex\u003d\u0027fake_builder_id\u0027)):"},{"line_number":1708,"context_line":"            ring.save(self.tmpfile)"},{"line_number":1709,"context_line":"        out, err \u003d self.run_srb(\u0027\u0027)"},{"line_number":1710,"context_line":"        self.assertOutputStub(out)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_4544871b","line":1707,"updated":"2017-04-26 03:37:58.000000000","message":"meh... I think this says anywhere we *don\u0027t* use create_sample_ring - but we *do* want to use assertOutputStub we have some coupling... or at a minimum have to duplicate the solution... hrmm...","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1d1818e38889d826b5d261ac631dc1e597eb791e","unresolved":false,"context_lines":[{"line_number":1704,"context_line":"                      })"},{"line_number":1705,"context_line":""},{"line_number":1706,"context_line":"        with mock.patch("},{"line_number":1707,"context_line":"                \u0027uuid.uuid4\u0027, return_value\u003dmock.Mock(hex\u003d\u0027fake_builder_id\u0027)):"},{"line_number":1708,"context_line":"            ring.save(self.tmpfile)"},{"line_number":1709,"context_line":"        out, err \u003d self.run_srb(\u0027\u0027)"},{"line_number":1710,"context_line":"        self.assertOutputStub(out)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_9ca3ef9e","line":1707,"in_reply_to":"5ff73747_4544871b","updated":"2017-04-26 17:00:28.000000000","message":"ok I think I see it - we can pass the builder id to assertOutputStub then have that method fix up the expected string to include the actual ring id, via template substitution.","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":1751,"context_line":"            \"          0 -100.00       \\n\" \\"},{"line_number":1752,"context_line":"            \"            3      3    3  127.0.0.4:6203 \" \\"},{"line_number":1753,"context_line":"            \"     127.0.0.4:6203  sdd4   0.00\" \\"},{"line_number":1754,"context_line":"            \"          0    0.00       \\n\" % (self.tmpfile, self.tmpfile)"},{"line_number":1755,"context_line":"        self.assertEqual(expected, mock_stdout.getvalue())"},{"line_number":1756,"context_line":""},{"line_number":1757,"context_line":"    def test_default_ringfile_check(self):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_e56dd398","line":1754,"updated":"2017-04-26 03:37:58.000000000","message":"this would be once place we could flip the id stuff around and validate the honest to goodness set by save w/o mock id is what gets output just by opening up the file and interpolating the _id into the expected output. \n\n... if we were worried about assertOutputStub being to loose with the actual id","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1d1818e38889d826b5d261ac631dc1e597eb791e","unresolved":false,"context_lines":[{"line_number":1751,"context_line":"            \"          0 -100.00       \\n\" \\"},{"line_number":1752,"context_line":"            \"            3      3    3  127.0.0.4:6203 \" \\"},{"line_number":1753,"context_line":"            \"     127.0.0.4:6203  sdd4   0.00\" \\"},{"line_number":1754,"context_line":"            \"          0    0.00       \\n\" % (self.tmpfile, self.tmpfile)"},{"line_number":1755,"context_line":"        self.assertEqual(expected, mock_stdout.getvalue())"},{"line_number":1756,"context_line":""},{"line_number":1757,"context_line":"    def test_default_ringfile_check(self):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_d0703ba8","line":1754,"in_reply_to":"5ff73747_e56dd398","updated":"2017-04-26 17:00:28.000000000","message":"Done","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"        self.assertOutputStub(out, builder_id\u003dring.id)"},{"line_number":1671,"context_line":""},{"line_number":1672,"context_line":"    def test_ipv6_output(self):"},{"line_number":1673,"context_line":"        ring \u003d RingBuilder(8, 3, 1)"},{"line_number":1674,"context_line":"        ring.add_dev({\u0027weight\u0027: 100.0,"},{"line_number":1675,"context_line":"                      \u0027region\u0027: 0,"},{"line_number":1676,"context_line":"                      \u0027zone\u0027: 0,"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_96032843","line":1673,"range":{"start_line":1673,"start_character":8,"end_line":1673,"end_character":12},"updated":"2017-05-18 08:56:51.000000000","message":"off-topic: some of \u0027ring\u0027 variable are actually \"ring_builder\"","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"}],"test/unit/cli/test_ringzipper.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"358cc6767f5669d23a72c4be45a235d93f0b84c6","unresolved":false,"context_lines":[{"line_number":81,"context_line":""},{"line_number":82,"context_line":"        return tmp_rings"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def assertDevices(self, zipped_ring, rings):"},{"line_number":85,"context_line":"        ring_devs \u003d []"},{"line_number":86,"context_line":"        for ring in rings:"},{"line_number":87,"context_line":"            ring_devs.extend(["}],"source_content_type":"text/x-python","patch_set":1,"id":"ba2be162_cb87f05e","line":84,"updated":"2017-03-06 14:10:36.000000000","message":"add more assertions for that zipped_ring.get_part_nodes will return concatenated primaries from each ring parts.","commit_id":"b1f032f10e624513a0373c09d747dc04983a3682"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"dafbef53ceaad09f6ade1753f485790f08f67a2e","unresolved":false,"context_lines":[{"line_number":81,"context_line":""},{"line_number":82,"context_line":"        return tmp_rings"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def assertDevices(self, zipped_ring, rings):"},{"line_number":85,"context_line":"        ring_devs \u003d []"},{"line_number":86,"context_line":"        for ring in rings:"},{"line_number":87,"context_line":"            ring_devs.extend(["}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_3f5d0dea","line":84,"in_reply_to":"ba2be162_cb87f05e","updated":"2017-03-07 08:19:50.000000000","message":"Done","commit_id":"b1f032f10e624513a0373c09d747dc04983a3682"}],"test/unit/common/ring/test_builder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":2123,"context_line":"        with self.assertRaises(AttributeError) as cm:"},{"line_number":2124,"context_line":"            rb.id"},{"line_number":2125,"context_line":"        self.assertIn(\u0027id attribute has not been initialised\u0027,"},{"line_number":2126,"context_line":"                      cm.exception.message)"},{"line_number":2127,"context_line":""},{"line_number":2128,"context_line":"    def test_search_devs(self):"},{"line_number":2129,"context_line":"        rb \u003d ring.RingBuilder(8, 3, 1)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_25229b53","line":2126,"updated":"2017-04-26 03:37:58.000000000","message":"the fallback on the id stuff is tested here","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":2119,"context_line":"                      cm.exception.message)"},{"line_number":2120,"context_line":"        # save must succeed for id to be assigned"},{"line_number":2121,"context_line":"        with self.assertRaises(IOError):"},{"line_number":2122,"context_line":"            rb.save(self.testdir + \u0027/non-existent-dir/foo.builder\u0027)"},{"line_number":2123,"context_line":"        with self.assertRaises(AttributeError) as cm:"},{"line_number":2124,"context_line":"            rb.id"},{"line_number":2125,"context_line":"        self.assertIn(\u0027id attribute has not been initialised\u0027,"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_d97d536f","line":2122,"updated":"2017-05-18 08:56:51.000000000","message":"nits) is it worth to use os.path.join as well as Line 2046?","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"}],"test/unit/common/ring/test_composite_builder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5ca4a467fedac4160ae8e716188c115f9dea3880","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        do_test(3)"},{"line_number":143,"context_line":"        do_test(4)"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"    def test_composite_no_buiderfile_info(self):"},{"line_number":146,"context_line":"        # this is possible scenario to feed the builder instance directly"},{"line_number":147,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":148,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a1ced50_b802fd07","line":145,"updated":"2017-03-23 20:31:34.000000000","message":"looks like this is now redundant, duplicates test_compose_rings","commit_id":"b727abd928f1bcfa9e76075ec11495905a5a5182"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"f2d108c6c117c476776fc9465eb6ba1eba03964b","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        do_test(3)"},{"line_number":143,"context_line":"        do_test(4)"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"    def test_composite_no_buiderfile_info(self):"},{"line_number":146,"context_line":"        # this is possible scenario to feed the builder instance directly"},{"line_number":147,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":148,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ffe62b97_30858aae","line":145,"in_reply_to":"1a1ced50_b802fd07","updated":"2017-03-24 09:46:27.000000000","message":"Oops, nice catch.","commit_id":"b727abd928f1bcfa9e76075ec11495905a5a5182"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"8d9c15abf756f4a4a742a9751550f799e04a5f5a","unresolved":false,"context_lines":[{"line_number":276,"context_line":"        self.assertEqual(got_ring._part_shift, 26)"},{"line_number":277,"context_line":"        self.assertDevices(got_ring, reverse_builders)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"        # but the composite rings are different order"},{"line_number":280,"context_line":"        # this order problem will be resolved in the future CLI tool"},{"line_number":281,"context_line":"        # which will keep consistent order in the composite builder file"},{"line_number":282,"context_line":"        # so currently this should not be a big problem"},{"line_number":283,"context_line":"        with self.assertRaises(AssertionError) as cm:"},{"line_number":284,"context_line":"            self.assertDevices(got_ring, builders)"},{"line_number":285,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ffe62b97_70b7e03b","line":282,"range":{"start_line":279,"start_character":0,"end_line":282,"end_character":55},"updated":"2017-03-29 01:41:40.000000000","message":"probably we should update the stale comment. Exactly we still keep the compose_rings behavior as same as this written but now we have write_composite_ring to attempt to assert the order consistency with existing ring.","commit_id":"2ffc288e26e76fd74b13ce171e646de25409532f"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            # remove last dev to simulate a ring with some history"},{"line_number":74,"context_line":"            builder.remove_dev(dev[\u0027id\u0027])"},{"line_number":75,"context_line":"            # add a dev that won\u0027t be assigned any parts"},{"line_number":76,"context_line":"            new_dev \u003d self.pop_region_device(dev[\u0027region\u0027])"},{"line_number":77,"context_line":"            new_dev[\u0027weight\u0027] \u003d 0"},{"line_number":78,"context_line":"            builder.add_dev(new_dev)"},{"line_number":79,"context_line":"            builder.rebalance()"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_c1580b32","line":76,"range":{"start_line":76,"start_character":45,"end_line":76,"end_character":58},"updated":"2017-04-12 05:20:37.000000000","message":"nits) this should be fine but using just \u0027region\u0027 variable for a new dev could be intuitive (which is same with Line 71.).","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"67efff9e6cbff6fecefb3ee58cec17e0ad4f270c","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            # remove last dev to simulate a ring with some history"},{"line_number":74,"context_line":"            builder.remove_dev(dev[\u0027id\u0027])"},{"line_number":75,"context_line":"            # add a dev that won\u0027t be assigned any parts"},{"line_number":76,"context_line":"            new_dev \u003d self.pop_region_device(dev[\u0027region\u0027])"},{"line_number":77,"context_line":"            new_dev[\u0027weight\u0027] \u003d 0"},{"line_number":78,"context_line":"            builder.add_dev(new_dev)"},{"line_number":79,"context_line":"            builder.rebalance()"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_fa9042ef","line":76,"range":{"start_line":76,"start_character":45,"end_line":76,"end_character":58},"in_reply_to":"bff0334d_c1580b32","updated":"2017-04-12 18:14:54.000000000","message":"ack, my bad","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":75,"context_line":"            # add a dev that won\u0027t be assigned any parts"},{"line_number":76,"context_line":"            new_dev \u003d self.pop_region_device(dev[\u0027region\u0027])"},{"line_number":77,"context_line":"            new_dev[\u0027weight\u0027] \u003d 0"},{"line_number":78,"context_line":"            builder.add_dev(new_dev)"},{"line_number":79,"context_line":"            builder.rebalance()"},{"line_number":80,"context_line":"            builder.save(fname)"},{"line_number":81,"context_line":"            self.assertTrue(os.path.exists(fname))"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_8139e3d7","line":78,"updated":"2017-04-12 05:20:37.000000000","message":"Do we need 0 weight dev always? I don\u0027t have a strong opinion but just wondering...","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"67efff9e6cbff6fecefb3ee58cec17e0ad4f270c","unresolved":false,"context_lines":[{"line_number":75,"context_line":"            # add a dev that won\u0027t be assigned any parts"},{"line_number":76,"context_line":"            new_dev \u003d self.pop_region_device(dev[\u0027region\u0027])"},{"line_number":77,"context_line":"            new_dev[\u0027weight\u0027] \u003d 0"},{"line_number":78,"context_line":"            builder.add_dev(new_dev)"},{"line_number":79,"context_line":"            builder.rebalance()"},{"line_number":80,"context_line":"            builder.save(fname)"},{"line_number":81,"context_line":"            self.assertTrue(os.path.exists(fname))"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_5aebae76","line":78,"in_reply_to":"bff0334d_8139e3d7","updated":"2017-04-12 18:14:54.000000000","message":"The goal is to have a device in the ring that will have no parts assigned to it and therefore not appear in r2p2d, to check that it\u0027s \u0027id\u0027 is still offset in the composite.","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    def test_compose_rings_rebalance_needed(self):"},{"line_number":275,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        # prepare another ring which has float replica count"},{"line_number":278,"context_line":"        dev \u003d self.pop_region_device(1)"},{"line_number":279,"context_line":"        builders[1].add_dev(dev)"},{"line_number":280,"context_line":"        self.assertTrue(builders[1].devs_changed)  # sanity check"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_efc09be3","line":277,"updated":"2017-04-12 05:20:37.000000000","message":"This comment is invalid. We need somehow:\n # prepare a ring with a new dev but not yet rebalanced.","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"67efff9e6cbff6fecefb3ee58cec17e0ad4f270c","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    def test_compose_rings_rebalance_needed(self):"},{"line_number":275,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        # prepare another ring which has float replica count"},{"line_number":278,"context_line":"        dev \u003d self.pop_region_device(1)"},{"line_number":279,"context_line":"        builders[1].add_dev(dev)"},{"line_number":280,"context_line":"        self.assertTrue(builders[1].devs_changed)  # sanity check"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_da79fec4","line":277,"in_reply_to":"bff0334d_efc09be3","updated":"2017-04-12 18:14:54.000000000","message":"yes, my mistake","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        self.assertIn(\"Problem with builders\", cm.exception.message)"},{"line_number":284,"context_line":"        self.assertIn(\"Builder needs rebalance\", cm.exception.message)"},{"line_number":285,"context_line":"        builders[1].rebalance()"},{"line_number":286,"context_line":"        compose_rings(builders)"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    def test_different_replica_count_works(self):"},{"line_number":289,"context_line":"        # create a ring builder"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_b729c537","line":286,"updated":"2017-04-12 05:20:37.000000000","message":"OK, this is for sanity.","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":369,"context_line":"        # make first version of composite ring"},{"line_number":370,"context_line":"        write_composite_ring(builders, self.output_ring)"},{"line_number":371,"context_line":"        # sanity"},{"line_number":372,"context_line":"        self.check_composite_ring(Ring(self.output_ring), builders[:2])"},{"line_number":373,"context_line":""},{"line_number":374,"context_line":"        # not ok to compose again with same *unchanged* builders"},{"line_number":375,"context_line":"        with self.assertRaises(ValueError) as cm:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_97fc8170","line":372,"range":{"start_line":372,"start_character":66,"end_line":372,"end_character":69},"updated":"2017-04-12 05:20:37.000000000","message":"Unnecessary index range.","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"eb5e7b9e3c41335640abfa3a20b978b083d197a3","unresolved":false,"context_lines":[{"line_number":374,"context_line":"        # not ok to compose again with same *unchanged* builders"},{"line_number":375,"context_line":"        with self.assertRaises(ValueError) as cm:"},{"line_number":376,"context_line":"            write_composite_ring(builders, self.output_ring)"},{"line_number":377,"context_line":"        error_lines \u003d cm.exception.message.split(\u0027\\n\u0027)"},{"line_number":378,"context_line":"        self.assertIn(\"None of the component builders has been modified\","},{"line_number":379,"context_line":"                      error_lines[0])"},{"line_number":380,"context_line":"        self.assertFalse(error_lines[1:])"},{"line_number":381,"context_line":""},{"line_number":382,"context_line":"        # not ok to use old versions of same builders"},{"line_number":383,"context_line":"        old_builders \u003d [copy.deepcopy(b) for b in builders]"}],"source_content_type":"text/x-python","patch_set":13,"id":"bff0334d_f71f4d69","line":380,"range":{"start_line":377,"start_character":0,"end_line":380,"end_character":41},"updated":"2017-04-12 05:20:37.000000000","message":"Perhaps, just\n\n self.assertEqual(\n     \"None of the component builders has been modified since the existing composite ring was built.\",\n     cm.exception.message)\n\nseems enough?","commit_id":"a80dce6e10d18d6edf63ed6f56e7dca025082c73"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0e17c24e4ca236fac9841c1f4b1a8265ce1547db","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            # remove last dev to simulate a ring with some history"},{"line_number":74,"context_line":"            builder.remove_dev(dev[\u0027id\u0027])"},{"line_number":75,"context_line":"            # add a dev that won\u0027t be assigned any parts"},{"line_number":76,"context_line":"            new_dev \u003d self.pop_region_device(dev[\u0027region\u0027])"},{"line_number":77,"context_line":"            new_dev[\u0027weight\u0027] \u003d 0"},{"line_number":78,"context_line":"            builder.add_dev(new_dev)"},{"line_number":79,"context_line":"            builder.rebalance()"}],"source_content_type":"text/x-python","patch_set":14,"id":"bff0334d_faa96223","line":76,"updated":"2017-04-12 18:14:45.000000000","message":"change dev[\u0027regio\u0027] to just region var","commit_id":"f304f207a90b4c9dcc2afde4dfa9e4f5b31efda7"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":58,"context_line":"        except OSError:"},{"line_number":59,"context_line":"            pass"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def save_builder_with_no_id(self, builder, fname):"},{"line_number":62,"context_line":"        orig_to_dict \u003d builder.to_dict"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"        def fake_to_dict():"},{"line_number":65,"context_line":"            res \u003d orig_to_dict()"},{"line_number":66,"context_line":"            res.pop(\u0027id\u0027)"},{"line_number":67,"context_line":"            return res"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        with mock.patch.object(builder, \u0027to_dict\u0027, fake_to_dict):"},{"line_number":70,"context_line":"            builder.save(fname)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    def save_builders(self, builders, missing_ids\u003dNone, prefix\u003d\u0027builder\u0027):"},{"line_number":73,"context_line":"        missing_ids \u003d missing_ids or []"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_39ed6fef","line":70,"range":{"start_line":61,"start_character":0,"end_line":70,"end_character":31},"updated":"2017-05-18 08:56:51.000000000","message":"nits) perhaps, this could be a helper function which can be used also in test_builder.py","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                dev_ids.append(dev[\u0027id\u0027])"},{"line_number":143,"context_line":"        self.assertEqual(len(builder_devs), len(dev_ids))"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"        def uniqueness(dev):"},{"line_number":146,"context_line":"            return (dev[\u0027ip\u0027], dev[\u0027port\u0027], dev[\u0027device\u0027])"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        # assert part assignment is ordered by ring order"},{"line_number":149,"context_line":"        part_count \u003d composite_ring.partition_count"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_79cd275c","line":146,"range":{"start_line":145,"start_character":0,"end_line":146,"end_character":58},"updated":"2017-05-18 08:56:51.000000000","message":"nits) uniqueness function can be used for Line 128 and Line 132.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":191,"context_line":"        self.assertEqual(expected_metadata, actual)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"class TestCompositeBuilder(BaseTestCompositeBuilder):"},{"line_number":195,"context_line":"    def test_compose_rings(self):"},{"line_number":196,"context_line":"        def do_test(builder_count):"},{"line_number":197,"context_line":"            builders \u003d self.create_sample_ringbuilders(builder_count)"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_1962ab3f","line":194,"range":{"start_line":194,"start_character":6,"end_line":194,"end_character":26},"updated":"2017-05-18 08:56:51.000000000","message":"nits) perhaps, sort of TestCompositeBuilderFunctions could be intuitive comparing with following TestCompositeRingBuilder class.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        # (default, part power is 6 with create_sample_ringbuilders)"},{"line_number":326,"context_line":"        builders \u003d self.create_sample_ringbuilders(1)"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        # prepare another ring which has different part power"},{"line_number":329,"context_line":"        builder \u003d RingBuilder(6, 1, 1)"},{"line_number":330,"context_line":"        _, fname \u003d tempfile.mkstemp(dir\u003dself.tmpdir)"},{"line_number":331,"context_line":"        for _ in range(4):"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_99b11b93","line":328,"range":{"start_line":328,"start_character":51,"end_line":328,"end_character":61},"updated":"2017-05-18 08:56:51.000000000","message":"not a part power but replica count.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def test_compose_with_builder_files(self):"},{"line_number":384,"context_line":"        cb_file \u003d os.path.join(self.tmpdir, \u0027test-composite-ring.json\u0027)"},{"line_number":385,"context_line":"        builders \u003d self.create_sample_ringbuilders(2)"},{"line_number":386,"context_line":"        cb \u003d CompositeRingBuilder(self.save_builders(builders))"},{"line_number":387,"context_line":"        cb.compose().save(self.output_ring)"},{"line_number":388,"context_line":"        self.check_composite_ring(self.output_ring, builders)"},{"line_number":389,"context_line":"        cb.save(cb_file)"},{"line_number":390,"context_line":""},{"line_number":391,"context_line":"        for i, b in enumerate(builders):"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_b9185f81","line":388,"range":{"start_line":386,"start_character":0,"end_line":388,"end_character":61},"updated":"2017-05-18 08:56:51.000000000","message":"nits) use self._make_composite_builder(builders)","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":422,"context_line":"                      cm.exception.message)"},{"line_number":423,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":424,"context_line":"        # ...unless we force it"},{"line_number":425,"context_line":"        cb.compose(force\u003dTrue).save(self.output_ring)"},{"line_number":426,"context_line":"        self.check_composite_ring(self.output_ring, builders)"},{"line_number":427,"context_line":"        # check composite builder persists ok again"},{"line_number":428,"context_line":"        cb_file \u003d os.path.join(self.tmpdir, \u0027test-composite-ring.json2\u0027)"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_3c9b1d73","line":425,"updated":"2017-05-18 08:56:51.000000000","message":"nits) forced compose run will increase the version, so explicitly self.assertEqual(2, cb.version) should work here.\n\nI notice same check also exists in check_composite_meta though.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":469,"context_line":"        # modify builders and save in different files"},{"line_number":470,"context_line":"        self.add_dev_and_rebalance(builders[1])"},{"line_number":471,"context_line":"        with self.assertRaises(ValueError):"},{"line_number":472,"context_line":"            cb.compose(builder_files)  # sanity check - originals are unchanged"},{"line_number":473,"context_line":"        other_files \u003d self.save_builders(builders, prefix\u003d\u0027other\u0027)"},{"line_number":474,"context_line":"        cb.compose(other_files).save(self.output_ring)"},{"line_number":475,"context_line":"        self.check_composite_ring(self.output_ring, builders)"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_bc396d42","line":472,"updated":"2017-05-18 08:56:51.000000000","message":"ok, add_dev_and_rebalance doesn\u0027t call save so the builder_files are not changed.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"a2298f86c71bca5ff656db002aca153a6bca9999","unresolved":false,"context_lines":[{"line_number":661,"context_line":"            self.assertIn(\"Attribute mismatch for id\", line)"},{"line_number":662,"context_line":"        self.assertEqual(1, cb.version)"},{"line_number":663,"context_line":""},{"line_number":664,"context_line":"    def test_compose_different_replica_count(self):"},{"line_number":665,"context_line":"        # not ok to change the number of replicas in a ring"},{"line_number":666,"context_line":"        builders \u003d self.create_sample_ringbuilders(3)"},{"line_number":667,"context_line":"        cb, builder_files \u003d self._make_composite_builder(builders)"}],"source_content_type":"text/x-python","patch_set":21,"id":"ff0f0b1f_bf871fd8","line":664,"range":{"start_line":664,"start_character":21,"end_line":664,"end_character":43},"updated":"2017-05-18 08:56:51.000000000","message":"nits) i prefer make the test name as test_compose_replica_count_changed regarding to similar name with another test case called test_compose_different_replica_count_works which tests different replica count for each component ring.","commit_id":"d40031b46fc0dda82fb027760ecda0c4a4b26ecf"}],"test/unit/common/ring/test_ring.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"32d01ab13d5aabc636aeaf411fe06fcbd68732a2","unresolved":false,"context_lines":[{"line_number":177,"context_line":"        test_metadata(None, {})"},{"line_number":178,"context_line":"        test_pattern \u003d (\u0027string\u0027, dict(), list(), {\u0027nest\u0027: {\u0027of\u0027: \u0027dict\u0027}})"},{"line_number":179,"context_line":"        for metadata in test_pattern:"},{"line_number":180,"context_line":"            test_metadata(metadata, metadata)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"class TestRing(TestRingBase):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_857ccfeb","line":180,"updated":"2017-04-26 03:37:58.000000000","message":"ok, so we can put anything we want onto ringdata now - cool","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1d1818e38889d826b5d261ac631dc1e597eb791e","unresolved":false,"context_lines":[{"line_number":177,"context_line":"        test_metadata(None, {})"},{"line_number":178,"context_line":"        test_pattern \u003d (\u0027string\u0027, dict(), list(), {\u0027nest\u0027: {\u0027of\u0027: \u0027dict\u0027}})"},{"line_number":179,"context_line":"        for metadata in test_pattern:"},{"line_number":180,"context_line":"            test_metadata(metadata, metadata)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"class TestRing(TestRingBase):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5ff73747_b0124f08","line":180,"in_reply_to":"5ff73747_857ccfeb","updated":"2017-04-26 17:00:28.000000000","message":"I\u0027m still not sure we have consensus on putting *any* metadata in the ring.gz, once we have the builder class and can write a composite.json [1]\n\n[]1  https://review.openstack.org/#/c/453827","commit_id":"6fa7a0391e659106f8f16caf828e5a8ebf800000"}]}