)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d57dc72ff17117ff1a5cc65bf338e42e186912bf","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"d2305351_48e38c42","updated":"2025-08-06 19:38:27.000000000","message":"\u003e I can look at squashing.\n\ntoo late now!  But I think we should try to respin/polish and merge.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"ab3af1bdb9c5a0ed1eaeb92e60a5385493625179","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"11f3f97c_e7fb7cd0","updated":"2025-08-05 06:49:28.000000000","message":"I can look at squashing.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4b00b7bde6de6d41ae524da2d06eca8a8c52734c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"87d11a54_e033b1c3","updated":"2025-08-07 19:01:02.000000000","message":"Thanks for thinking about this Tim!\n\nI\u0027m having trouble deciding if I should think you support the unification (simplification?) of maintaining the invariant of `calc_dev_id_bytes(len(devs)) \u003c\u003d r2p2d[0].itemsize` into a single function used consistently by add_device \u0026 remove_failed_devices?\n\nI don\u0027t think you\u0027ve weighted in explicitly?  Should I assume you support this change in spirit based on your seeming willing-ness to squash it pre-merge or perhaps your comments should suggest to me the whole idea reads to you as more \"WTF? why?\" - If you have a an opinion can you clarify it for me?\n\nI\u0027m not going to be able to make much progress on this until next week I\u0027m afraid - I\u0027ll also try to solicit some input from Matt while you\u0027re out.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"4f304e465ffab8dc3e6411361889fc04f9653ff8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"9d954acf_c9c5569b","updated":"2025-08-01 02:41:59.000000000","message":"Very interesting, although I don\u0027t have the confidence to land it.\nThe meat is actually the extra checks, not just refactoring, as I understand.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b246b2807cca85ee14be7e12bd44cf7895ad7919","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"82d82cbc_419d8364","in_reply_to":"87d11a54_e033b1c3","updated":"2025-08-07 23:06:29.000000000","message":"\u003e  Should I assume you support this change in spirit based on your seeming willing-ness to squash it pre-merge\n\nI tend to err toward \"This dev whose opinion I generally trust went to the effort to write this up -- it\u0027s probably an improvement, or at least has the kernel of a good idea.\" If it makes the thing more legible/maintainable, let\u0027s do it!\n\n\u003e or perhaps your comments should suggest to me the whole idea reads to you as more \"WTF? why?\"\n\nI was mostly just surprised when an idea that sounded like it would want one new helper ended up needing *three*, separated by ~900 lines. Then I realized that I could completely neuter one of them and no tests broke:\n```\ndiff --git a/swift/common/ring/builder.py b/swift/common/ring/builder.py\nindex a090b003b..2b34c4778 100644\n--- a/swift/common/ring/builder.py\n+++ b/swift/common/ring/builder.py\n@@ -169,9 +169,6 @@ class RingBuilder(object):\n         \"\"\"\n         Check that the dev list length and r2p2d array itemsize are consistent.\n         \"\"\"\n-        if self._bytes_needed_for_dev_list() !\u003d self.dev_id_bytes:\n-            raise exceptions.RingValidationError(\n-                \"dev_id_bytes is not consistent with the dev list\")\n \n     def set_dev_id_bytes(self, new_dev_id_bytes):\n         if self._replica2part2dev:\n```\n\n\u003e If you have a an opinion can you clarify it for me?\n\nIt doesn\u0027t really matter much to me. Make sure there\u0027s width enough when we\u0027re growing, see if we can narrow when we\u0027re shrinking; have a new function to make sure we\u0027re the right width and call it both when we\u0027re growing and shrinking; same difference to me. If we like it more, merge it! We can always try to beat it into something that we like *even better* later.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"}],"swift/common/ring/builder.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"ab3af1bdb9c5a0ed1eaeb92e60a5385493625179","unresolved":true,"context_lines":[{"line_number":165,"context_line":"            return self._bytes_needed_for_dev_list()"},{"line_number":166,"context_line":"        return self._replica2part2dev[0].itemsize"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _check_dev_list_and_r2p2d_array_invariant(self):"},{"line_number":169,"context_line":"        \"\"\""},{"line_number":170,"context_line":"        Check that the dev list length and r2p2d array itemsize are consistent."},{"line_number":171,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"ab74ada0_40af06a7","line":168,"updated":"2025-08-05 06:49:28.000000000","message":"This just has the one caller -- why not inline it as part of `_enforce_dev_list_and_r2p2d_array_invariant`?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d57dc72ff17117ff1a5cc65bf338e42e186912bf","unresolved":true,"context_lines":[{"line_number":165,"context_line":"            return self._bytes_needed_for_dev_list()"},{"line_number":166,"context_line":"        return self._replica2part2dev[0].itemsize"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _check_dev_list_and_r2p2d_array_invariant(self):"},{"line_number":169,"context_line":"        \"\"\""},{"line_number":170,"context_line":"        Check that the dev list length and r2p2d array itemsize are consistent."},{"line_number":171,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"8d81342c_f04480a5","line":168,"in_reply_to":"ab74ada0_40af06a7","updated":"2025-08-06 19:38:27.000000000","message":"I think the other caller is in tests?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d57dc72ff17117ff1a5cc65bf338e42e186912bf","unresolved":true,"context_lines":[{"line_number":1069,"context_line":"        while self.devs and self.devs[-1] is None:"},{"line_number":1070,"context_line":"            self.devs.pop()"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":"        # XXX could this be done in a more obvious way?"},{"line_number":1073,"context_line":"        if self.dev_id_bytes \u003e 2:"},{"line_number":1074,"context_line":"            # Consider shrinking the r2p2d array itemsize themselves"},{"line_number":1075,"context_line":"            new_dev_id_bytes \u003d self.dev_id_bytes // 2"}],"source_content_type":"text/x-python","patch_set":1,"id":"27f1e512_33e1cd37","line":1072,"updated":"2025-08-06 19:38:27.000000000","message":"Tim, I\u0027d really appreciate if you could help think of how we might spell this elegantly?\n\nI feel like the method should \"just\": check what sort of dev_id_bytes would be appropriate for the devs list ... and fix the array?  With maybe some special handling for for down-sizing.\n\nIs that sort of what it\u0027s already doing?  Is that obvious from a quick read of it?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"05b327033d68e8b5072500507a7d1a32d3cb7762","unresolved":true,"context_lines":[{"line_number":1069,"context_line":"        while self.devs and self.devs[-1] is None:"},{"line_number":1070,"context_line":"            self.devs.pop()"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":"        # XXX could this be done in a more obvious way?"},{"line_number":1073,"context_line":"        if self.dev_id_bytes \u003e 2:"},{"line_number":1074,"context_line":"            # Consider shrinking the r2p2d array itemsize themselves"},{"line_number":1075,"context_line":"            new_dev_id_bytes \u003d self.dev_id_bytes // 2"}],"source_content_type":"text/x-python","patch_set":1,"id":"4c2df2ee_67fe2ee4","line":1072,"in_reply_to":"27f1e512_33e1cd37","updated":"2025-08-07 17:20:47.000000000","message":"There\u0027s definitely an opportunity to simplify since we axed support for `dev_id_bytes \u003d 1`. Maybe something like\n```\nif devs:\n    if self.dev_id_bytes \u003d\u003d 2 and len(self.devs) \u003e self.max_dev_id:\n        self.set_dev_id_bytes(4)\n    elif self.dev_id_bytes \u003d\u003d 4 and len(self.devs) \u003c 2 ** 15:\n        # Only shrink if the IDs all fit in the lower half of the next size\n        # down; this avoids excess churn when adding/removing devices near\n        # the limit of a particular dev_id_bytes\n        self.set_dev_id_bytes(2)\n```","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4b00b7bde6de6d41ae524da2d06eca8a8c52734c","unresolved":true,"context_lines":[{"line_number":1069,"context_line":"        while self.devs and self.devs[-1] is None:"},{"line_number":1070,"context_line":"            self.devs.pop()"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":"        # XXX could this be done in a more obvious way?"},{"line_number":1073,"context_line":"        if self.dev_id_bytes \u003e 2:"},{"line_number":1074,"context_line":"            # Consider shrinking the r2p2d array itemsize themselves"},{"line_number":1075,"context_line":"            new_dev_id_bytes \u003d self.dev_id_bytes // 2"}],"source_content_type":"text/x-python","patch_set":1,"id":"e3b24f8d_688de098","line":1072,"in_reply_to":"4c2df2ee_67fe2ee4","updated":"2025-08-07 19:01:02.000000000","message":"that does read SO much better!  Does the hidden else clause end up being a `RingValidationError(\u0027wtf is dev_id_bytes returning now?\u0027)`\n\nplease feel free to push over.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"ab3af1bdb9c5a0ed1eaeb92e60a5385493625179","unresolved":true,"context_lines":[{"line_number":1143,"context_line":"                        part2dev.append(self.none_dev_id)"},{"line_number":1144,"context_line":"                        new_parts +\u003d 1"},{"line_number":1145,"context_line":"                elif len(part2dev) \u003e desired_length:"},{"line_number":1146,"context_line":"                    # Too long: truncate this mapping."},{"line_number":1147,"context_line":"                    for part in range(desired_length, len(part2dev)):"},{"line_number":1148,"context_line":"                        dev_losing_part \u003d self.devs[part2dev[part]]"},{"line_number":1149,"context_line":"                        dev_losing_part[\u0027parts\u0027] -\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"9b8ce09c_9c7c43c1","line":1146,"updated":"2025-08-05 06:49:28.000000000","message":"Should we have a similar check here?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d57dc72ff17117ff1a5cc65bf338e42e186912bf","unresolved":true,"context_lines":[{"line_number":1143,"context_line":"                        part2dev.append(self.none_dev_id)"},{"line_number":1144,"context_line":"                        new_parts +\u003d 1"},{"line_number":1145,"context_line":"                elif len(part2dev) \u003e desired_length:"},{"line_number":1146,"context_line":"                    # Too long: truncate this mapping."},{"line_number":1147,"context_line":"                    for part in range(desired_length, len(part2dev)):"},{"line_number":1148,"context_line":"                        dev_losing_part \u003d self.devs[part2dev[part]]"},{"line_number":1149,"context_line":"                        dev_losing_part[\u0027parts\u0027] -\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"684ffa85_94e91d22","line":1146,"in_reply_to":"9b8ce09c_9c7c43c1","updated":"2025-08-06 19:38:27.000000000","message":"I think yu mean in L1148 `part2dev[part]` \"could\" return the none_dev_id - yes I think that should be a bug and could raise a `RingValidationError`","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"}],"test/unit/common/ring/test_builder.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"05b327033d68e8b5072500507a7d1a32d3cb7762","unresolved":true,"context_lines":[{"line_number":2806,"context_line":"        rb \u003d ring.RingBuilder(8, 2, 1)"},{"line_number":2807,"context_line":"        rb.add_dev({\u0027id\u0027: 0, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2808,"context_line":"                    \u0027port\u0027: 6200, \u0027weight\u0027: 1.0, \u0027device\u0027: \u0027sda\u0027})"},{"line_number":2809,"context_line":"        rb.add_dev({\u0027id\u0027: 1, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2810,"context_line":"                    \u0027port\u0027: 6200, \u0027device\u0027: \u0027sdb\u0027, \u0027weight\u0027: 2000})"},{"line_number":2811,"context_line":"        self.assertEqual(2, rb.dev_id_bytes)"},{"line_number":2812,"context_line":"        new_id \u003d 2 ** 16 - 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"a4cc9118_68694c30","line":2809,"range":{"start_line":2809,"start_character":26,"end_line":2809,"end_character":27},"updated":"2025-08-07 17:20:47.000000000","message":"Stronger test if this were `2 ** 16 - 2`","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4b00b7bde6de6d41ae524da2d06eca8a8c52734c","unresolved":true,"context_lines":[{"line_number":2806,"context_line":"        rb \u003d ring.RingBuilder(8, 2, 1)"},{"line_number":2807,"context_line":"        rb.add_dev({\u0027id\u0027: 0, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2808,"context_line":"                    \u0027port\u0027: 6200, \u0027weight\u0027: 1.0, \u0027device\u0027: \u0027sda\u0027})"},{"line_number":2809,"context_line":"        rb.add_dev({\u0027id\u0027: 1, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2810,"context_line":"                    \u0027port\u0027: 6200, \u0027device\u0027: \u0027sdb\u0027, \u0027weight\u0027: 2000})"},{"line_number":2811,"context_line":"        self.assertEqual(2, rb.dev_id_bytes)"},{"line_number":2812,"context_line":"        new_id \u003d 2 ** 16 - 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"468e0480_192a1841","line":2809,"range":{"start_line":2809,"start_character":26,"end_line":2809,"end_character":27},"in_reply_to":"a4cc9118_68694c30","updated":"2025-08-07 19:01:02.000000000","message":"i think a lot of values would work here - I see `test_wide_device_limits` uses `new_id \u003d 2 ** 16 - 2` and asserts `itemsize, 2` - I think using the larger dev_id would be more consistent with the surrounding tests.  Consistency in tests is a kind of strength.  Variety of tests is another kind of strength.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"05b327033d68e8b5072500507a7d1a32d3cb7762","unresolved":true,"context_lines":[{"line_number":2813,"context_line":"        rb.add_dev({\u0027id\u0027: new_id, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2814,"context_line":"                    \u0027port\u0027: 6200, \u0027weight\u0027: 1.0, \u0027device\u0027: \u0027sdc\u0027})"},{"line_number":2815,"context_line":"        self.assertEqual(4, rb.dev_id_bytes)"},{"line_number":2816,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2817,"context_line":"        self.assertEqual(rb._replica2part2dev, None)"},{"line_number":2818,"context_line":"        rb.rebalance()"},{"line_number":2819,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"}],"source_content_type":"text/x-python","patch_set":1,"id":"1ee041bf_7c5568f1","line":2816,"updated":"2025-08-07 17:20:47.000000000","message":"I\u0027m not clear on what this gets us that the `self.assertEqual(4, rb.dev_id_bytes)` doesn\u0027t.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4b00b7bde6de6d41ae524da2d06eca8a8c52734c","unresolved":true,"context_lines":[{"line_number":2813,"context_line":"        rb.add_dev({\u0027id\u0027: new_id, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2814,"context_line":"                    \u0027port\u0027: 6200, \u0027weight\u0027: 1.0, \u0027device\u0027: \u0027sdc\u0027})"},{"line_number":2815,"context_line":"        self.assertEqual(4, rb.dev_id_bytes)"},{"line_number":2816,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2817,"context_line":"        self.assertEqual(rb._replica2part2dev, None)"},{"line_number":2818,"context_line":"        rb.rebalance()"},{"line_number":2819,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"}],"source_content_type":"text/x-python","patch_set":1,"id":"44ac4019_7d47aa68","line":2816,"in_reply_to":"1ee041bf_7c5568f1","updated":"2025-08-07 19:01:02.000000000","message":"IIUC dev_id_bytes is a property that has one of two behaviors:\nreturns the \"calulated dev_id_bytes based off the lenght of the dev list\"\nOR\nreturns the itemsize of the array\n\nIMHO a unittest should almost never assert \"either\" of those things \"indirectly\" IF it\u0027s trying to assert one or the other of them explicitly.\n\nI think what this \"gets us\" is a clear spelling of a pattern when a test wants to assert *both*","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b246b2807cca85ee14be7e12bd44cf7895ad7919","unresolved":true,"context_lines":[{"line_number":2813,"context_line":"        rb.add_dev({\u0027id\u0027: new_id, \u0027region\u0027: 0, \u0027zone\u0027: 0, \u0027ip\u0027: \u0027127.0.0.1\u0027,"},{"line_number":2814,"context_line":"                    \u0027port\u0027: 6200, \u0027weight\u0027: 1.0, \u0027device\u0027: \u0027sdc\u0027})"},{"line_number":2815,"context_line":"        self.assertEqual(4, rb.dev_id_bytes)"},{"line_number":2816,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2817,"context_line":"        self.assertEqual(rb._replica2part2dev, None)"},{"line_number":2818,"context_line":"        rb.rebalance()"},{"line_number":2819,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"}],"source_content_type":"text/x-python","patch_set":1,"id":"f2160f1d_40f40d61","line":2816,"in_reply_to":"44ac4019_7d47aa68","updated":"2025-08-07 23:06:29.000000000","message":"But the pattern for that seems to be\n\n- assert on size of `dev_id_bytes`\n- assert on state of `_replica2part2dev`\n\nThen we\u0027ve got the `_check_dev_list_and_r2p2d_array_invariant` call in between -- I feel like I\u0027m still missing something.","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b246b2807cca85ee14be7e12bd44cf7895ad7919","unresolved":true,"context_lines":[{"line_number":2818,"context_line":"        rb.rebalance()"},{"line_number":2819,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2820,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 4)"},{"line_number":2821,"context_line":"        rb.remove_dev(new_id)"},{"line_number":2822,"context_line":"        rb.rebalance()"},{"line_number":2823,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2824,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 2)"}],"source_content_type":"text/x-python","patch_set":1,"id":"de8da734_08f48c1c","line":2821,"updated":"2025-08-07 23:06:29.000000000","message":"Maybe more compelling with a\n```\nself.assertEqual(4, rb.dev_id_bytes)\nself.assertEqual(rb._replica2part2dev[0].itemsize, 4)\n```\nhere between `remove_dev` and `rebalance`? To show that it\u0027s the rebalance that causes the resize?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d57dc72ff17117ff1a5cc65bf338e42e186912bf","unresolved":true,"context_lines":[{"line_number":2820,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 4)"},{"line_number":2821,"context_line":"        rb.remove_dev(new_id)"},{"line_number":2822,"context_line":"        rb.rebalance()"},{"line_number":2823,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2824,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 2)"},{"line_number":2825,"context_line":""},{"line_number":2826,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3a3ec2f4_3cfc1392","line":2823,"updated":"2025-08-06 19:38:27.000000000","message":"other caller of check","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"05b327033d68e8b5072500507a7d1a32d3cb7762","unresolved":true,"context_lines":[{"line_number":2820,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 4)"},{"line_number":2821,"context_line":"        rb.remove_dev(new_id)"},{"line_number":2822,"context_line":"        rb.rebalance()"},{"line_number":2823,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2824,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 2)"},{"line_number":2825,"context_line":""},{"line_number":2826,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"6ad817fb_e1b61582","line":2823,"in_reply_to":"3a3ec2f4_3cfc1392","updated":"2025-08-07 17:20:47.000000000","message":"OK, but why? I guess to demonstrate that `add_dev` and `rebalance` *do* preserve this invariant... except that we haven\u0027t demonstrated anywhere that `_check_dev_list_and_r2p2d_array_invariant` can detect breakage :-(\n\nOr are we saying we expect out-of-tree ring-management software might call this, too? \u0027Cause the leading underscore says otherwise.\n\nWhy are we only doing this here in this one test? Why not just do it explicitly in the end of `rebalance`, similar to hod `add_dev` explicitly calls `_enforce_dev_list_and_r2p2d_array_invariant` just before returning (which in turn ends with the call to `_check_dev_list_and_r2p2d_array_invariant`)?","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4b00b7bde6de6d41ae524da2d06eca8a8c52734c","unresolved":true,"context_lines":[{"line_number":2820,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 4)"},{"line_number":2821,"context_line":"        rb.remove_dev(new_id)"},{"line_number":2822,"context_line":"        rb.rebalance()"},{"line_number":2823,"context_line":"        rb._check_dev_list_and_r2p2d_array_invariant()"},{"line_number":2824,"context_line":"        self.assertEqual(rb._replica2part2dev[0].itemsize, 2)"},{"line_number":2825,"context_line":""},{"line_number":2826,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"b6684373_670dbdfc","line":2823,"in_reply_to":"6ad817fb_e1b61582","updated":"2025-08-07 19:01:02.000000000","message":"\u003e except that we haven\u0027t demonstrated anywhere that _check_dev_list_and_r2p2d_array_invariant can detect breakage\n\nI think that would be a strict improvement to this change - i\u0027d love some help adding that.\n\n\u003e Cause the leading underscore says otherwise.\n\nI would hope out-of-tree ring-management software can mostly ignore (and rely) on this internal-to-the-builder invariant.  Hopefully they understand the need to call `add_device` and the other helpers instead of mangling `.devs` directly.\n\n\u003e in this one test\n\nthe lack of additional testing is not a feature of this patch\n\n\u003e do it explicitly in the end of rebalance\n\nthat sounds reasonable (if perhaps redundant) to me!","commit_id":"d872c8d74df4a96d3e3e9c85cc0c84dd214b2be8"}]}