)]}'
{"swift/common/ring/builder.py":[{"author":{"_account_id":2622,"name":"Samuel Merritt","email":"spam+launchpad@andcheese.org","username":"torgomatic"},"change_message_id":"ae3414ef7d30392333fdacf9ad415cbc2513e300","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"            overweight_dev_replica.sort("},{"line_number":1094,"context_line":"                key\u003dlambda dr: dr[0][\u0027parts_wanted\u0027])"},{"line_number":1095,"context_line":"            for dev, replica in overweight_dev_replica:"},{"line_number":1096,"context_line":"                if all(replica_plan[tier][\u0027min\u0027] \u003c\u003d"},{"line_number":1097,"context_line":"                       replicas_at_tier[tier] \u003c"},{"line_number":1098,"context_line":"                       replica_plan[tier][\u0027max\u0027]"},{"line_number":1099,"context_line":"                       for tier in dev[\u0027tiers\u0027]):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_2175ae64","line":1096,"updated":"2017-07-17 23:03:16.000000000","message":"I think, instead of this change, it would be better to remove this conditional entirely, as there are many rings for which it will never be true.\n\nConsider a 1-region ring. The replica plan for the region will have min\u003d3, max\u003d3, and replicas_at_tier[the_region] \u003d 3. Since the region is in dev[\u0027tiers\u0027] for all devices, this conditional will never be true.\n\nWhen I delete this conditional entirely from this change set and run the tests, everything passes.\n\nUnfortunately, I don\u0027t currently have a good-enough understanding to know if this change is good or not.","commit_id":"9154a67f4ae90bdb3c1bcc1da4bac792337cb4af"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"bab2c3e49369a45ccdf0b2d2a3da3096c811ab8d","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"            overweight_dev_replica.sort("},{"line_number":1094,"context_line":"                key\u003dlambda dr: dr[0][\u0027parts_wanted\u0027])"},{"line_number":1095,"context_line":"            for dev, replica in overweight_dev_replica:"},{"line_number":1096,"context_line":"                if all(replica_plan[tier][\u0027min\u0027] \u003c\u003d"},{"line_number":1097,"context_line":"                       replicas_at_tier[tier] \u003c"},{"line_number":1098,"context_line":"                       replica_plan[tier][\u0027max\u0027]"},{"line_number":1099,"context_line":"                       for tier in dev[\u0027tiers\u0027]):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_bec84574","line":1096,"in_reply_to":"1f1a1f67_2175ae64","updated":"2017-08-08 07:05:47.000000000","message":"Good catch. I also confirmed the tests passed if removing the condition here.\n\nAnd then, if we don\u0027t have to care about the replica plan, probably removing _gather_parts_for_balance_can_disperse method and using _gather_parts_for_balance_forced instead like [1] may be simple because the difference between can_disperse and forced is just this condition here. \n\n1: https://gist.github.com/bloodeagle40234/4e4d097cfde14cf3eba8b8c28ad9e2b3","commit_id":"9154a67f4ae90bdb3c1bcc1da4bac792337cb4af"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"7dee92243be5341495d1b8ce4c9f7bec75c3c1d1","unresolved":false,"context_lines":[{"line_number":1093,"context_line":"            overweight_dev_replica.sort("},{"line_number":1094,"context_line":"                key\u003dlambda dr: dr[0][\u0027parts_wanted\u0027])"},{"line_number":1095,"context_line":"            for dev, replica in overweight_dev_replica:"},{"line_number":1096,"context_line":"                if all(replica_plan[tier][\u0027min\u0027] \u003c\u003d"},{"line_number":1097,"context_line":"                       replicas_at_tier[tier] \u003c"},{"line_number":1098,"context_line":"                       replica_plan[tier][\u0027max\u0027]"},{"line_number":1099,"context_line":"                       for tier in dev[\u0027tiers\u0027]):"},{"line_number":1100,"context_line":"                    continue"},{"line_number":1101,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_57691b23","line":1098,"range":{"start_line":1096,"start_character":0,"end_line":1098,"end_character":48},"updated":"2017-08-09 07:30:41.000000000","message":"I found if we change this condition like:\n\n+                # if and only if minimum number of replica(s) exists\n+                # in the tier, we don\u0027t have to move the part so skip\n+                # here to find another candidate\n+                if any(replica_plan[tier][\u0027min\u0027] \u003d\u003d\n+                       replicas_at_tier[tier]\n\nwe can keep the behavior Clay wanted and the new test on my case passed correctly but it looks to break something on  test.unit.common.ring.test_ring.TestRing.test_get_more_nodes unexpectedly that doesn\u0027t make me sense why it has been broken yet.","commit_id":"9154a67f4ae90bdb3c1bcc1da4bac792337cb4af"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"47dadef8093d440fa5623dfcde832e6bea88b794","unresolved":false,"context_lines":[{"line_number":1133,"context_line":""},{"line_number":1134,"context_line":"        self._gather_parts_for_balance_can_disperse("},{"line_number":1135,"context_line":"            assign_parts, start, replica_plan)"},{"line_number":1136,"context_line":"        if not assign_parts:"},{"line_number":1137,"context_line":"            self._gather_parts_for_balance_forced(assign_parts, start)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    def _gather_parts_for_balance_forced(self, assign_parts, start, **kwargs):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_8de1c601","line":1136,"updated":"2017-08-09 20:51:04.000000000","message":"perhaps the thrust of the issue is *this* conditional?  The code is *trying* to give preference to parts that should disperse (the \"any\" conditional is trying to skip parts that are \"fitted\" according to the replica plan and find parts which are unfit).\n\nThe assumption in *this* conditional is that if we find *any* unfitted parts, we should *not* perform a less restrictive gather.  Perhaps the heuristic here is insufficient!  Did we find \"enough\" parts?  How many times have we looked?  Maybe the conditional in _can_disperse is is not restrictive *enough* and we think we\u0027re gathering parts that can/should disperse but really we should give up on that noise and just aim for better balance within the tier!?\n\nI don\u0027t think the correct answer is to remove the preference for rebalance of undispersed parts and focus instead *only* on balance.  I\u0027m not sure why the unittests don\u0027t do a better job of demonstrating why this preference is desirable and necessary.  But without it - I think a bunch of *existing* replica2part2dev tables are left with terrible dispersion.","commit_id":"9154a67f4ae90bdb3c1bcc1da4bac792337cb4af"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"8dcc2b7d2c10cf0bd7ee81571071c6e8b5204f9b","unresolved":false,"context_lines":[{"line_number":1133,"context_line":""},{"line_number":1134,"context_line":"        self._gather_parts_for_balance_can_disperse("},{"line_number":1135,"context_line":"            assign_parts, start, replica_plan)"},{"line_number":1136,"context_line":"        if not assign_parts:"},{"line_number":1137,"context_line":"            self._gather_parts_for_balance_forced(assign_parts, start)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    def _gather_parts_for_balance_forced(self, assign_parts, start, **kwargs):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f436f4f_bcbb6de3","line":1136,"in_reply_to":"9f436f4f_8de1c601","updated":"2017-08-21 06:52:48.000000000","message":"What I can tell you right now is those, in my test case, 1) gather_parts_for_balance_can_disperse found reassign_parts, 2) those reassign_parts unfortunately already in the devices that has large parts_wanted values, 3) RingBuilder is forced to push back to the original device which release the partition because it\u0027s only the device in the same failure domain (i.e. a zone with high parts_wanted)\n\nTo consider that, right now, we can not decide the partition to be gathered only from replica_plan, IMO. If we could cut into the code deeply, possibley\n\n1. _gather_parts_xxx reports the info where devices the partitions are from\n2. _reassign_parts refer the info and avoid to push the partitions to the original device \n3. and then, _gather_parts considers previous rebalance failure to pick up the reassign_parts heuristically\n\nFor item 1, 2. I can make the code easily [1] because I already has done with the debugging to know the behavior. On the item 3,I\u0027m not sure on this algorithms can work as well and it could be my challenge to mess the ring rebalance algorithm.\n\nI added this topic into PTG etherpard [2] so I\u0027 like to keep our attention and define possible solution(s?) in the Queen cycle. Thanks!\n   \n\n1: https://gist.github.com/bloodeagle40234/8b5333179cbdb014b2e3184df8db29d4\n\n2:  https://etherpad.openstack.org/p/swift-ptg-queens","commit_id":"9154a67f4ae90bdb3c1bcc1da4bac792337cb4af"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04409b4dcdc8e983bd9fe50357f4d3cef205f83e","unresolved":false,"context_lines":[{"line_number":1277,"context_line":"                    # Choose the roomiest tier among those that don\u0027t"},{"line_number":1278,"context_line":"                    # already have their max replicas assigned according"},{"line_number":1279,"context_line":"                    # to the replica_plan."},{"line_number":1280,"context_line":"                    candidates \u003d [t for t in tier2children[tier] if"},{"line_number":1281,"context_line":"                                  replicas_at_tier[t] \u003c"},{"line_number":1282,"context_line":"                                  replica_plan[t][\u0027max\u0027]]"},{"line_number":1283,"context_line":""},{"line_number":1284,"context_line":"                    if not candidates:"},{"line_number":1285,"context_line":"                        raise Exception(\u0027no home for %s/%s %s\u0027 % ("}],"source_content_type":"text/x-python","patch_set":3,"id":"9f436f4f_c6f556cd","line":1282,"range":{"start_line":1280,"start_character":0,"end_line":1282,"end_character":57},"updated":"2017-08-09 13:34:08.000000000","message":"One more thing, I\u0027m looking at with skeptical eyes is here.\n\nIn fact, this candidate list doesn\u0027t care about parts_available_in_tier (actually, it\u0027s parts_wanted_in_tier IMHO) so a candidate can be remained with no parts_wanted (i.e. 0 and the tier may be the tier that gave the part as assign_part)\n\nHowever, if we avoid the candidate here like:\n\n candidates \u003d [t for t in tier2children[tier] if\n                         replicas_at_tier[t] \u003c\n                         replica_plan[t][\u0027max\u0027]\n                         and parts_available_in_tier[t])]\n\nthat will result into an exception below so that I messed gather_parts logic in this patch.","commit_id":"248eecd26d97f765ad78cbda31ee3c46a9b73aa4"}],"test/unit/common/ring/test_builder.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04409b4dcdc8e983bd9fe50357f4d3cef205f83e","unresolved":false,"context_lines":[{"line_number":2437,"context_line":"            (0, 0, \u0027127.0.0.1\u0027, 3): [0, 256, 0, 0],"},{"line_number":2438,"context_line":"        })"},{"line_number":2439,"context_line":""},{"line_number":2440,"context_line":"    def test_ring_builder_moves_only_surplas_parts(self):"},{"line_number":2441,"context_line":"        # [Scenario]"},{"line_number":2442,"context_line":"        # 3 replicas"},{"line_number":2443,"context_line":"        # Initial condition"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f436f4f_66bc6adc","line":2440,"updated":"2017-08-09 13:34:08.000000000","message":"This is from Clay\u0027s suggestion in the review comment.\n\nAnd this test failed if we uses previous patch set but patch set 3 passed as I expected.","commit_id":"248eecd26d97f765ad78cbda31ee3c46a9b73aa4"}],"test/unit/common/ring/test_ring.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04409b4dcdc8e983bd9fe50357f4d3cef205f83e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"9f436f4f_4633a6ac","updated":"2017-08-09 13:34:08.000000000","message":"I suppose the change in this file is ok because test_get_more_nodes is so brittle since the statement is from hard-coded. I walked through the statements in the test and it looks reasonable if I understand the logic correctly.","commit_id":"248eecd26d97f765ad78cbda31ee3c46a9b73aa4"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"04409b4dcdc8e983bd9fe50357f4d3cef205f83e","unresolved":false,"context_lines":[{"line_number":659,"context_line":"        # N.B. the first handoffs can be changed more than num_parts_changed"},{"line_number":660,"context_line":"        # when the first one results in same device in the primary of the part"},{"line_number":661,"context_line":"        # after rebalance."},{"line_number":662,"context_line":"        self.assertGreaterEqual(changed_first_handoff, num_parts_changed)"},{"line_number":663,"context_line":""},{"line_number":664,"context_line":"        # Test"},{"line_number":665,"context_line":"        part, devs \u003d r.get_nodes(\u0027a\u0027, \u0027c\u0027, \u0027o\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f436f4f_26dd4242","line":662,"updated":"2017-08-09 13:34:08.000000000","message":"Perhaps, other assertions for changed_first_handoff can be changed as we ll as here if the note I added would be correct.","commit_id":"248eecd26d97f765ad78cbda31ee3c46a9b73aa4"}]}