)]}'
{"swift/obj/reconstructor.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ad0e7bd24a905888065ffae5ec8bc0d33c744f9d","unresolved":false,"context_lines":[{"line_number":620,"context_line":"        # is one of the handoff locations - which *might* be us already!"},{"line_number":621,"context_line":"        dest_nodes \u003d itertools.chain("},{"line_number":622,"context_line":"            job[\u0027sync_to\u0027],"},{"line_number":623,"context_line":"            job[\u0027policy\u0027].object_ring.get_more_nodes(job[\u0027partition\u0027]),"},{"line_number":624,"context_line":"        )"},{"line_number":625,"context_line":"        syncd_with \u003d 0"},{"line_number":626,"context_line":"        reverted_objs \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_96da9477","line":623,"updated":"2015-04-20 12:24:23.000000000","message":"But why push a partition from one handoff to another handoff? That\u0027s all this does I think?","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ade58a5912260d851f6fb5c11602876b8f91f93b","unresolved":false,"context_lines":[{"line_number":620,"context_line":"        # is one of the handoff locations - which *might* be us already!"},{"line_number":621,"context_line":"        dest_nodes \u003d itertools.chain("},{"line_number":622,"context_line":"            job[\u0027sync_to\u0027],"},{"line_number":623,"context_line":"            job[\u0027policy\u0027].object_ring.get_more_nodes(job[\u0027partition\u0027]),"},{"line_number":624,"context_line":"        )"},{"line_number":625,"context_line":"        syncd_with \u003d 0"},{"line_number":626,"context_line":"        reverted_objs \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_812c794b","line":623,"in_reply_to":"dac4157f_96da9477","updated":"2015-04-20 17:59:05.000000000","message":"after a rebalance we might not be in the handoff chain, or we might be a handoff way out in left field beyond the reach of the proxy where the primary handoff would be the next best place besides the correct primary location.","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ad0e7bd24a905888065ffae5ec8bc0d33c744f9d","unresolved":false,"context_lines":[{"line_number":628,"context_line":"            if \u0027index\u0027 not in node:"},{"line_number":629,"context_line":"                # this handoff node is filling in for the primary node for"},{"line_number":630,"context_line":"                # which this job was constructed"},{"line_number":631,"context_line":"                node[\u0027index\u0027] \u003d job[\u0027frag_index\u0027]"},{"line_number":632,"context_line":"            if syncd_with \u003e\u003d len(job[\u0027sync_to\u0027]):"},{"line_number":633,"context_line":"                break"},{"line_number":634,"context_line":"            if node[\u0027id\u0027] \u003d\u003d job[\u0027local_dev\u0027][\u0027id\u0027]:"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_d0ca544f","line":631,"updated":"2015-04-20 12:24:23.000000000","message":"Huh, I thought I\u0027d checked this in ec review but sure enough there\u0027s the call to get_more_nodes above :/","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ade58a5912260d851f6fb5c11602876b8f91f93b","unresolved":false,"context_lines":[{"line_number":628,"context_line":"            if \u0027index\u0027 not in node:"},{"line_number":629,"context_line":"                # this handoff node is filling in for the primary node for"},{"line_number":630,"context_line":"                # which this job was constructed"},{"line_number":631,"context_line":"                node[\u0027index\u0027] \u003d job[\u0027frag_index\u0027]"},{"line_number":632,"context_line":"            if syncd_with \u003e\u003d len(job[\u0027sync_to\u0027]):"},{"line_number":633,"context_line":"                break"},{"line_number":634,"context_line":"            if node[\u0027id\u0027] \u003d\u003d job[\u0027local_dev\u0027][\u0027id\u0027]:"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_61d84554","line":631,"in_reply_to":"dac4157f_d0ca544f","updated":"2015-04-20 17:59:05.000000000","message":"yeah so it\u0027s a keyerror not to set node index.","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"41966012e5c46676ad1b8fe1601045d19604d02f","unresolved":false,"context_lines":[{"line_number":573,"context_line":"                n for n in"},{"line_number":574,"context_line":"                job[\u0027policy\u0027].object_ring.get_part_nodes(job[\u0027partition\u0027])"},{"line_number":575,"context_line":"                if n[\u0027id\u0027] !\u003d job[\u0027local_dev\u0027][\u0027id\u0027] and"},{"line_number":576,"context_line":"                n[\u0027id\u0027] not in (n[\u0027id\u0027] for n in job[\u0027sync_to\u0027])"},{"line_number":577,"context_line":"            ],"},{"line_number":578,"context_line":"        )"},{"line_number":579,"context_line":"        syncd_with \u003d 0"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_a1d52b44","line":576,"updated":"2015-04-23 22:18:17.000000000","message":"I know that minimal understanding of Python is required to be a core reviewer, bu I cannot remember Python scoping rules well enough to make sure n does not leak out of (n[\u0027id\u0027] for n in foo()).  Maybe make them n and m?","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d108469f7db794da1c833342f8458e37ebf3ddd8","unresolved":false,"context_lines":[{"line_number":573,"context_line":"                n for n in"},{"line_number":574,"context_line":"                job[\u0027policy\u0027].object_ring.get_part_nodes(job[\u0027partition\u0027])"},{"line_number":575,"context_line":"                if n[\u0027id\u0027] !\u003d job[\u0027local_dev\u0027][\u0027id\u0027] and"},{"line_number":576,"context_line":"                n[\u0027id\u0027] not in (n[\u0027id\u0027] for n in job[\u0027sync_to\u0027])"},{"line_number":577,"context_line":"            ],"},{"line_number":578,"context_line":"        )"},{"line_number":579,"context_line":"        syncd_with \u003d 0"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_9bb5e432","line":576,"in_reply_to":"bab921fb_a1d52b44","updated":"2015-04-24 03:22:22.000000000","message":"oh i think it does leak, but the order of operations probably leaves it correct - be way better to use a different identifier there for clarity tho - nice catch\n\n#willfix","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"}],"test/probe/common.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ad0e7bd24a905888065ffae5ec8bc0d33c744f9d","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        if os.path.ismount(device):"},{"line_number":319,"context_line":"            os.system(\u0027sudo umount %s\u0027 % device)"},{"line_number":320,"context_line":"        else:"},{"line_number":321,"context_line":"            renamer(device, device + \"X\")"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":"    def revive_drive(self, device):"},{"line_number":324,"context_line":"        disabled_name \u003d device + \"X\""}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_b31da568","line":321,"updated":"2015-04-20 12:24:23.000000000","message":"might it be worth maintaining a list of killed drives and have the tearDown method revive them all? Otherwise, can I be left with a broken cluster if a probe test fails ? (yeah, I almost always run resetswift but I\u0027m fallible)","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ade58a5912260d851f6fb5c11602876b8f91f93b","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        if os.path.ismount(device):"},{"line_number":319,"context_line":"            os.system(\u0027sudo umount %s\u0027 % device)"},{"line_number":320,"context_line":"        else:"},{"line_number":321,"context_line":"            renamer(device, device + \"X\")"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":"    def revive_drive(self, device):"},{"line_number":324,"context_line":"        disabled_name \u003d device + \"X\""}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_01cf4980","line":321,"in_reply_to":"dac4157f_b31da568","updated":"2015-04-20 17:59:05.000000000","message":"yeah it\u0027s pretty important to me when developing and testing probetests that after a failure the probetests leave thing in the broken state (i.e. don\u0027t clean up after yourself)\n\nIt maybe possible to have both with an option, or cleanup that only runs on success - it will certainly be easier to manage as more of the machinery for breaking things moves to common bases - but we\u0027ll also continue to invent new ways to break things so a resetswift may also be the best teardown.","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"41966012e5c46676ad1b8fe1601045d19604d02f","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        if os.path.isdir(disabled_name):"},{"line_number":326,"context_line":"            renamer(device + \"X\", device)"},{"line_number":327,"context_line":"        else:"},{"line_number":328,"context_line":"            os.system(\u0027sudo mount %s\u0027 % device)"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":""},{"line_number":331,"context_line":"class ReplProbeTest(ProbeTest):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_52497b8a","line":328,"updated":"2015-04-23 22:18:17.000000000","message":"I don\u0027t understand the need of sudo here. If mount is done without arguments, then it must have a line in /etc/fstab. But if so, you might as well add \"user\" option there and not require sudo.\n\nI don\u0027t have sudo installed on most of my systems, so this kinda jumps out as an interesting change... Not that I run probtests often.\n\n(I didn\u0027t notice this code at its old location)","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d108469f7db794da1c833342f8458e37ebf3ddd8","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        if os.path.isdir(disabled_name):"},{"line_number":326,"context_line":"            renamer(device + \"X\", device)"},{"line_number":327,"context_line":"        else:"},{"line_number":328,"context_line":"            os.system(\u0027sudo mount %s\u0027 % device)"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":""},{"line_number":331,"context_line":"class ReplProbeTest(ProbeTest):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_9b684489","line":328,"in_reply_to":"bab921fb_52497b8a","updated":"2015-04-24 03:22:22.000000000","message":"so yeah, pre-existing code just relocated.\n\nBut I\u0027m not even sure what you\u0027re suggesting be fixed?  Does this question apply to the umount line as well?  Those commands don\u0027t work on my system without sudo - even if there is a line in /etc/fstab.\n\nFWIW, this branch only applies if you\u0027re using mount_check \u003d true - which is basically everyone except notmyname - so we\u0027ll probably have to make any changes to this code in a separate patch and have him run it.","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"}],"test/probe/test_reconstructor_rebuild.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ad0e7bd24a905888065ffae5ec8bc0d33c744f9d","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                         self.direct_get(failed_primary, opart))"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        # just to be nice"},{"line_number":223,"context_line":"        self.revive_drive(device_path)"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"if __name__ \u003d\u003d \"__main__\":"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_d04b742a","line":223,"updated":"2015-04-20 12:24:23.000000000","message":"could do this in a finally clause to be really nice, or in tearDown()","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ade58a5912260d851f6fb5c11602876b8f91f93b","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                         self.direct_get(failed_primary, opart))"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        # just to be nice"},{"line_number":223,"context_line":"        self.revive_drive(device_path)"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"if __name__ \u003d\u003d \"__main__\":"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_01a4e9a3","line":223,"in_reply_to":"dac4157f_d04b742a","updated":"2015-04-20 17:59:05.000000000","message":"this seems like the most stright-forward way to make it conditional on success.  I need the device to remain broken after a failure for troubleshooting.  It is really *just* to be nice.","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"}],"test/probe/test_reconstructor_revert.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ad0e7bd24a905888065ffae5ec8bc0d33c744f9d","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        else:"},{"line_number":396,"context_line":"            self.fail(\u0027Unable to find all the handoff fragments!\u0027)"},{"line_number":397,"context_line":""},{"line_number":398,"context_line":"        # with primary nodes offline the handoffs will hold their fragments"},{"line_number":399,"context_line":"        for etag, node in handoff_fragments:"},{"line_number":400,"context_line":"            self.reconstructor.once(number\u003dself.config_number(node))"},{"line_number":401,"context_line":"            self.assertEqual(etag, self.direct_get(node, opart))"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_d0d80ff2","line":398,"updated":"2015-04-20 12:24:23.000000000","message":"Well, in the case of the second handoff, it will attempt to push to the first handoff, only to have ssync fail due to a 409 when it discovers that the first handoff already has another fragment. Which seems like a whole lot of wasted effort.\n\n(If i cherry pick https://review.openstack.org/#/c/174875/ onto this review then this test fails here because the revert succeeds)","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ade58a5912260d851f6fb5c11602876b8f91f93b","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        else:"},{"line_number":396,"context_line":"            self.fail(\u0027Unable to find all the handoff fragments!\u0027)"},{"line_number":397,"context_line":""},{"line_number":398,"context_line":"        # with primary nodes offline the handoffs will hold their fragments"},{"line_number":399,"context_line":"        for etag, node in handoff_fragments:"},{"line_number":400,"context_line":"            self.reconstructor.once(number\u003dself.config_number(node))"},{"line_number":401,"context_line":"            self.assertEqual(etag, self.direct_get(node, opart))"}],"source_content_type":"text/x-python","patch_set":1,"id":"dac4157f_34260d69","line":398,"in_reply_to":"dac4157f_d0d80ff2","updated":"2015-04-20 17:59:05.000000000","message":"yes I wrote this test as a reaction to my realization that the revert handling under failure would lead to an unreasonable \"piling\" of fragments on the first handoff (unless - as it turns out - we don\u0027t allow ssyc to place multiple fragment on the same node simultaneously)\n\nMy ideas was to try to draw out a scenario, and think about the behavior - if you agree with this tests behavior - you can\u0027t agree with the timestamp change.\n\nBut that doesn\u0027t mean it\u0027s an either/or!  More likely we need a both!\n\nI agree the 409 isn\u0027t efficient, but I\u0027m not sure a pre-flight REPLICATE suffix hash check forcing the target handoff to rehash all these suffixes that he isn\u0027t even supposed to have is better.  My bigger concern is if the 409 is an un-handled failure that prevents any other progress in the suffix.","commit_id":"b8b4d3499bb83d8c0cd296728141ba555dc0fe9f"}],"test/unit/obj/test_reconstructor.py":[{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"41966012e5c46676ad1b8fe1601045d19604d02f","unresolved":false,"context_lines":[{"line_number":976,"context_line":"    def test_process_job_all_timeout(self):"},{"line_number":977,"context_line":"        self.reconstructor._reset_stats()"},{"line_number":978,"context_line":"        with mock_ssync_sender():"},{"line_number":979,"context_line":"            with nested(mocked_http_conn(*[Timeout()] * 8)):"},{"line_number":980,"context_line":"                found_jobs \u003d []"},{"line_number":981,"context_line":"                for part_info in self.reconstructor.collect_parts():"},{"line_number":982,"context_line":"                    jobs \u003d self.reconstructor.build_reconstruction_jobs("}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_c042563e","line":979,"updated":"2015-04-23 22:18:17.000000000","message":"This looks entirely too subtle to allow.","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"b1e250881d37a7f0e25cf4c79cb67c90cffd4642","unresolved":false,"context_lines":[{"line_number":976,"context_line":"    def test_process_job_all_timeout(self):"},{"line_number":977,"context_line":"        self.reconstructor._reset_stats()"},{"line_number":978,"context_line":"        with mock_ssync_sender():"},{"line_number":979,"context_line":"            with nested(mocked_http_conn(*[Timeout()] * 8)):"},{"line_number":980,"context_line":"                found_jobs \u003d []"},{"line_number":981,"context_line":"                for part_info in self.reconstructor.collect_parts():"},{"line_number":982,"context_line":"                    jobs \u003d self.reconstructor.build_reconstruction_jobs("}],"source_content_type":"text/x-python","patch_set":3,"id":"3acd31a7_6706bc87","line":979,"in_reply_to":"bab921fb_3bac989f","updated":"2015-05-04 20:30:39.000000000","message":"Never mind me, I need to learn Python harder","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d108469f7db794da1c833342f8458e37ebf3ddd8","unresolved":false,"context_lines":[{"line_number":976,"context_line":"    def test_process_job_all_timeout(self):"},{"line_number":977,"context_line":"        self.reconstructor._reset_stats()"},{"line_number":978,"context_line":"        with mock_ssync_sender():"},{"line_number":979,"context_line":"            with nested(mocked_http_conn(*[Timeout()] * 8)):"},{"line_number":980,"context_line":"                found_jobs \u003d []"},{"line_number":981,"context_line":"                for part_info in self.reconstructor.collect_parts():"},{"line_number":982,"context_line":"                    jobs \u003d self.reconstructor.build_reconstruction_jobs("}],"source_content_type":"text/x-python","patch_set":3,"id":"bab921fb_3bac989f","line":979,"in_reply_to":"bab921fb_c042563e","updated":"2015-04-24 03:22:22.000000000","message":"heh, too subtle to... \"allow\"?\n\nThese tests suck - I hate them, but haven\u0027t managed to get rid of all of them (yet?)\n\nThe reason it changed from 10 to 8 is because we took two calls out of the job (the ones we were making to ourselves).\n\nI\u0027m not sure if that helps address your issue here or not.  But given the change being correct these tests have to change :D","commit_id":"4983b0621cd1b7396204a882bb2cf5791265d254"}]}