)]}'
{"tests/base.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"917d0915bfb64be9238e789415d0ec06b0390151","unresolved":false,"context_lines":[{"line_number":3198,"context_line":"        )"},{"line_number":3199,"context_line":""},{"line_number":3200,"context_line":"    def all(self):"},{"line_number":3201,"context_line":"        return self.inState()"},{"line_number":3202,"context_line":""},{"line_number":3203,"context_line":""},{"line_number":3204,"context_line":"class RecordingMergeClient(zuul.merger.client.MergeClient):"}],"source_content_type":"text/x-python","patch_set":2,"id":"6cc4ca7a_d825ab27","line":3201,"updated":"2021-07-26 20:48:53.000000000","message":"This should use _test_getMergeJobsInState to avoid races.","commit_id":"1d07501bb65bd9dba11eef7310cac15a20efd79d"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"917d0915bfb64be9238e789415d0ec06b0390151","unresolved":false,"context_lines":[{"line_number":5205,"context_line":""},{"line_number":5206,"context_line":"    def __areAllMergeJobsWaiting(self, matcher) -\u003e bool:"},{"line_number":5207,"context_line":"        # Look up the queued merge jobs directly from ZooKeeper"},{"line_number":5208,"context_line":"        queued_merge_jobs \u003d list(self.merger_api._test_getMergeJobsInState())"},{"line_number":5209,"context_line":"        # Always ignore merge jobs which are on hold"},{"line_number":5210,"context_line":"        for job in queued_merge_jobs:"},{"line_number":5211,"context_line":"            if job.state !\u003d MergeRequest.HOLD:"}],"source_content_type":"text/x-python","patch_set":2,"id":"110fdbb4_a49ffb1d","line":5208,"updated":"2021-07-26 20:48:53.000000000","message":"When you switch all() to use _test_getMergeJobsInState, you can switch this to use all()","commit_id":"1d07501bb65bd9dba11eef7310cac15a20efd79d"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2ed52e1211c5ddc29bdce1efdb94bd3b07c892d7","unresolved":false,"context_lines":[{"line_number":5214,"context_line":""},{"line_number":5215,"context_line":"    def __areAllMergeJobsWaiting(self, matcher) -\u003e bool:"},{"line_number":5216,"context_line":"        # Look up the queued merge jobs directly from ZooKeeper"},{"line_number":5217,"context_line":"        queued_merge_jobs \u003d list(self.merger_api._test_getMergeJobsInState())"},{"line_number":5218,"context_line":"        # Always ignore merge jobs which are on hold"},{"line_number":5219,"context_line":"        for job in queued_merge_jobs:"},{"line_number":5220,"context_line":"            if job.state !\u003d MergeRequest.HOLD:"}],"source_content_type":"text/x-python","patch_set":5,"id":"c6fcab82_385394ed","line":5217,"updated":"2021-07-29 01:23:49.000000000","message":"Can use .all() here","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":4961,"context_line":"        self.executor_server.hold_jobs_in_build \u003d False"},{"line_number":4962,"context_line":"        self.executor_server.release()"},{"line_number":4963,"context_line":"        self.scheds.execute(lambda app: app.sched.executor.stop())"},{"line_number":4964,"context_line":"        self.scheds.execute(lambda app: app.sched.merger.stop())"},{"line_number":4965,"context_line":"        if self.merge_server:"},{"line_number":4966,"context_line":"            self.merge_server.stop()"},{"line_number":4967,"context_line":"            self.merge_server.join()"}],"source_content_type":"text/x-python","patch_set":13,"id":"6f9dce00_a81f0fbd","side":"PARENT","line":4964,"updated":"2021-08-05 18:54:35.000000000","message":"For anyone wondering why this goes away the scheduler doesn\u0027t run a merger client thread anymore. Do we need to clean up the executor.stop() just above for similar reasons?","commit_id":"770a610f925b223c82224938e25236d6ccd77e0f"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":4961,"context_line":"        self.executor_server.hold_jobs_in_build \u003d False"},{"line_number":4962,"context_line":"        self.executor_server.release()"},{"line_number":4963,"context_line":"        self.scheds.execute(lambda app: app.sched.executor.stop())"},{"line_number":4964,"context_line":"        self.scheds.execute(lambda app: app.sched.merger.stop())"},{"line_number":4965,"context_line":"        if self.merge_server:"},{"line_number":4966,"context_line":"            self.merge_server.stop()"},{"line_number":4967,"context_line":"            self.merge_server.join()"}],"source_content_type":"text/x-python","patch_set":13,"id":"00414a64_45e86b4a","side":"PARENT","line":4964,"updated":"2021-08-05 22:26:08.000000000","message":"Yes we should -- it\u0027s currently a no-op.","commit_id":"770a610f925b223c82224938e25236d6ccd77e0f"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":5127,"context_line":"    def merge_job_history(self):"},{"line_number":5128,"context_line":"        history \u003d {}"},{"line_number":5129,"context_line":"        for app in self.scheds:"},{"line_number":5130,"context_line":"            history.update(app.sched.merger.merger_api.history)"},{"line_number":5131,"context_line":"        return history"},{"line_number":5132,"context_line":""},{"line_number":5133,"context_line":"    @merge_job_history.deleter"}],"source_content_type":"text/x-python","patch_set":13,"id":"08e3655a_845cba78","line":5130,"updated":"2021-08-05 22:26:08.000000000","message":"I think the keys of the history are job uuids.  Since this is a merger_api history, see HoldableMergerApi.submit for where it\u0027s appended.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":5127,"context_line":"    def merge_job_history(self):"},{"line_number":5128,"context_line":"        history \u003d {}"},{"line_number":5129,"context_line":"        for app in self.scheds:"},{"line_number":5130,"context_line":"            history.update(app.sched.merger.merger_api.history)"},{"line_number":5131,"context_line":"        return history"},{"line_number":5132,"context_line":""},{"line_number":5133,"context_line":"    @merge_job_history.deleter"}],"source_content_type":"text/x-python","patch_set":13,"id":"dfdbfdef_99a90a7b","line":5130,"updated":"2021-08-05 18:54:35.000000000","message":"Since the keys of the history dict are job types I think the last sched\u0027s history will win and overwrite the others. I suspect we want to do some sort of appending of all the type history lists instead.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3b6ac25ca3e670fccbca88bbab20a7676fc47558","unresolved":false,"context_lines":[{"line_number":5127,"context_line":"    def merge_job_history(self):"},{"line_number":5128,"context_line":"        history \u003d {}"},{"line_number":5129,"context_line":"        for app in self.scheds:"},{"line_number":5130,"context_line":"            history.update(app.sched.merger.merger_api.history)"},{"line_number":5131,"context_line":"        return history"},{"line_number":5132,"context_line":""},{"line_number":5133,"context_line":"    @merge_job_history.deleter"}],"source_content_type":"text/x-python","patch_set":13,"id":"d59847e1_9f1729d9","line":5130,"in_reply_to":"08e3655a_845cba78","updated":"2021-08-05 22:54:02.000000000","message":"Yup I think app.sched.merger has the type based history via RecordingMergeClient but app.sched.merger.merger_api is HoldableMergerApi.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"}],"tests/unit/test_configloader.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"f1f225404cd326adeff6eb811977f541a42de76a","unresolved":false,"context_lines":[{"line_number":327,"context_line":"        # Check that only one merger:cat job was requested"},{"line_number":328,"context_line":"        # org/project1 and org/project2 have an empty load_classes"},{"line_number":329,"context_line":"        cat_jobs \u003d [job for job in self.merge_job_history.values()"},{"line_number":330,"context_line":"                    if job.job_type \u003d\u003d MergeRequest.CAT]"},{"line_number":331,"context_line":"        self.assertEqual(1, len(cat_jobs))"},{"line_number":332,"context_line":"        old_layout \u003d tenant.layout"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"6c48e54b_52601e06","line":330,"updated":"2021-07-31 20:00:26.000000000","message":"Disregard this comment.  I stand corrected; this is the merger api history not the merger client history.  It\u0027s possible those no longer need to be distinct.  But we don\u0027t need to change that in this patch.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"6c748c8d9a8f4bd5aa07e482ae0d24611255b1dc","unresolved":false,"context_lines":[{"line_number":327,"context_line":"        # Check that only one merger:cat job was requested"},{"line_number":328,"context_line":"        # org/project1 and org/project2 have an empty load_classes"},{"line_number":329,"context_line":"        cat_jobs \u003d [job for job in self.merge_job_history.values()"},{"line_number":330,"context_line":"                    if job.job_type \u003d\u003d MergeRequest.CAT]"},{"line_number":331,"context_line":"        self.assertEqual(1, len(cat_jobs))"},{"line_number":332,"context_line":"        old_layout \u003d tenant.layout"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"20d344ea_531a63a9","line":330,"updated":"2021-08-02 14:11:36.000000000","message":"It\u0027s probably worth a commit message note, yes.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2ed52e1211c5ddc29bdce1efdb94bd3b07c892d7","unresolved":false,"context_lines":[{"line_number":327,"context_line":"        # Check that only one merger:cat job was requested"},{"line_number":328,"context_line":"        # org/project1 and org/project2 have an empty load_classes"},{"line_number":329,"context_line":"        cat_jobs \u003d [job for job in self.merge_job_history.values()"},{"line_number":330,"context_line":"                    if job.job_type \u003d\u003d MergeRequest.CAT]"},{"line_number":331,"context_line":"        self.assertEqual(1, len(cat_jobs))"},{"line_number":332,"context_line":"        old_layout \u003d tenant.layout"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"02030583_5202eb76","line":330,"updated":"2021-07-29 01:23:49.000000000","message":"This could be:\n  cat_jobs \u003d self.merge_job_history[MergeRequest.CAT]\n\nThere\u0027s a subtle difference between these: the old is getting all of the merge jobs from any source, the new is getting all of the merge jobs submitted by the schedulers.  They should be equivalent since only the schedulers should submit merge jobs.  But it\u0027s a difference to be aware of (perhaps in the case of a test that submits a merge job directly).","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"b6162956fb6d004d86a2d23c4c9fe8f9a8f42346","unresolved":false,"context_lines":[{"line_number":327,"context_line":"        # Check that only one merger:cat job was requested"},{"line_number":328,"context_line":"        # org/project1 and org/project2 have an empty load_classes"},{"line_number":329,"context_line":"        cat_jobs \u003d [job for job in self.merge_job_history.values()"},{"line_number":330,"context_line":"                    if job.job_type \u003d\u003d MergeRequest.CAT]"},{"line_number":331,"context_line":"        self.assertEqual(1, len(cat_jobs))"},{"line_number":332,"context_line":"        old_layout \u003d tenant.layout"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"36aa4990_ad836d7e","line":330,"in_reply_to":"02030583_5202eb76","updated":"2021-08-02 08:11:46.000000000","message":"I have removed the RecordingMergeClient.history in a later change to combine both history approaches: https://review.opendev.org/c/zuul/zuul/+/773030/9\n\nRegarding the subtle difference: That\u0027s right, but I didn\u0027t find a better solution to this. Should we add a comment somewhere (maybe in the commit message, as it\u0027s bound to this change)?","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"}],"tests/unit/test_merger_repo.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"        # Create a fake lost merge request.  This is based on"},{"line_number":1022,"context_line":"        # test_lost_merge_requests in test_zk."},{"line_number":1023,"context_line":"        merger_api \u003d MergerApi(self.zk_client)"},{"line_number":1024,"context_line":""},{"line_number":1025,"context_line":"        payload \u003d {\u0027merge\u0027: \u0027test\u0027}"},{"line_number":1026,"context_line":"        merger_api.submit("}],"source_content_type":"text/x-python","patch_set":13,"id":"82f3a661_cce03ec6","line":1023,"updated":"2021-08-05 18:54:35.000000000","message":"Do we use a distinct zk_client object here? Wonder why it wasn\u0027t stopped above.\n\nAbove in the diff we stop the executor servers merger via a more merger specific method without touching the zk_client. Would that be a better approach?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"        # Create a fake lost merge request.  This is based on"},{"line_number":1022,"context_line":"        # test_lost_merge_requests in test_zk."},{"line_number":1023,"context_line":"        merger_api \u003d MergerApi(self.zk_client)"},{"line_number":1024,"context_line":""},{"line_number":1025,"context_line":"        payload \u003d {\u0027merge\u0027: \u0027test\u0027}"},{"line_number":1026,"context_line":"        merger_api.submit("}],"source_content_type":"text/x-python","patch_set":13,"id":"23b65291_d8b3ffd2","line":1023,"updated":"2021-08-05 22:26:08.000000000","message":"Okay, it\u0027s harder to fully stop and start the executor than it is to do this.  I\u0027d like to just leave it as is since it works.\n\n(Detail: you can\u0027t start the executor after stopping it, and the test shutdown stops it automatically.  So we\u0027d need a different base class to handle this.)","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"9c019af94dd4d825de463f46a170f3e9e94f5ed0","unresolved":false,"context_lines":[{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"        # Create a fake lost merge request.  This is based on"},{"line_number":1022,"context_line":"        # test_lost_merge_requests in test_zk."},{"line_number":1023,"context_line":"        merger_api \u003d MergerApi(self.zk_client)"},{"line_number":1024,"context_line":""},{"line_number":1025,"context_line":"        payload \u003d {\u0027merge\u0027: \u0027test\u0027}"},{"line_number":1026,"context_line":"        merger_api.submit("}],"source_content_type":"text/x-python","patch_set":13,"id":"07a392ed_a6ea6aec","line":1023,"updated":"2021-08-05 23:27:11.000000000","message":"That looks okay too.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"        # Create a fake lost merge request.  This is based on"},{"line_number":1022,"context_line":"        # test_lost_merge_requests in test_zk."},{"line_number":1023,"context_line":"        merger_api \u003d MergerApi(self.zk_client)"},{"line_number":1024,"context_line":""},{"line_number":1025,"context_line":"        payload \u003d {\u0027merge\u0027: \u0027test\u0027}"},{"line_number":1026,"context_line":"        merger_api.submit("}],"source_content_type":"text/x-python","patch_set":13,"id":"f91c9d33_51d1e295","line":1023,"updated":"2021-08-05 20:42:11.000000000","message":"Yes, the executor server creates its own zk client connection.  We could probably shut down the whole executor for this test.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3b6ac25ca3e670fccbca88bbab20a7676fc47558","unresolved":false,"context_lines":[{"line_number":1020,"context_line":""},{"line_number":1021,"context_line":"        # Create a fake lost merge request.  This is based on"},{"line_number":1022,"context_line":"        # test_lost_merge_requests in test_zk."},{"line_number":1023,"context_line":"        merger_api \u003d MergerApi(self.zk_client)"},{"line_number":1024,"context_line":""},{"line_number":1025,"context_line":"        payload \u003d {\u0027merge\u0027: \u0027test\u0027}"},{"line_number":1026,"context_line":"        merger_api.submit("}],"source_content_type":"text/x-python","patch_set":13,"id":"4ae4e0a7_4932d4f0","line":1023,"in_reply_to":"23b65291_d8b3ffd2","updated":"2021-08-05 22:54:02.000000000","message":"Does it make sense to stop the merger in the Executor more similarly to how we do it elsewhere? Lines 903-905 of this file for example.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"}],"tests/unit/test_scheduler.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2ed52e1211c5ddc29bdce1efdb94bd3b07c892d7","unresolved":false,"context_lines":[{"line_number":555,"context_line":"        for f in list(merger_gear.functions.keys()):"},{"line_number":556,"context_line":"            f \u003d f.decode(\u0027utf8\u0027)"},{"line_number":557,"context_line":"            if f.startswith(\u0027merger:\u0027):"},{"line_number":558,"context_line":"                merger_gear.unRegisterFunction(f)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":"        self.create_branch(\u0027org/project\u0027, \u0027stable\u0027)"},{"line_number":561,"context_line":"        self.fake_gerrit.addEvent("}],"source_content_type":"text/x-python","patch_set":5,"id":"afb7815b_46e9ef51","side":"PARENT","line":558,"updated":"2021-07-29 01:23:49.000000000","message":"Just because we can optimize this, it doesn\u0027t mean we should.  When making a change like this which doesn\u0027t appear to be strictly necessary, it\u0027s a good practice to include your reasoning in the commit message.  Like \"we can remove the secondary merger from test_branch_deletion because...\".\n\nHowever, I don\u0027t think we can.  I used git blame to look for the original reasoning for this, and the reason is mentioned in the commit message.  See da5bb7eea65776ea7f953ae1476e06d9062eaaab\n\nOf course, it should also have been a code comment, but we deal with what we have.\n\nI think you should restructure this to keep the secondary merger (while disabling the executor merger).  I think you have done that in other tests.","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"1b4f1c133c66a1a64549473b9a619c4901795f8f","unresolved":false,"context_lines":[{"line_number":555,"context_line":"        for f in list(merger_gear.functions.keys()):"},{"line_number":556,"context_line":"            f \u003d f.decode(\u0027utf8\u0027)"},{"line_number":557,"context_line":"            if f.startswith(\u0027merger:\u0027):"},{"line_number":558,"context_line":"                merger_gear.unRegisterFunction(f)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":"        self.create_branch(\u0027org/project\u0027, \u0027stable\u0027)"},{"line_number":561,"context_line":"        self.fake_gerrit.addEvent("}],"source_content_type":"text/x-python","patch_set":5,"id":"8c6a9f8c_f9e2ad3c","side":"PARENT","line":558,"in_reply_to":"a00007a5_fc538d5e","updated":"2021-07-30 05:58:10.000000000","message":"Update on my last comment (somehow this sentence went missing): But now I see that I understood this part wrong and the test is intended to do so.","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2be7e430417b6e49645f24337bb08088711e42c2","unresolved":false,"context_lines":[{"line_number":555,"context_line":"        for f in list(merger_gear.functions.keys()):"},{"line_number":556,"context_line":"            f \u003d f.decode(\u0027utf8\u0027)"},{"line_number":557,"context_line":"            if f.startswith(\u0027merger:\u0027):"},{"line_number":558,"context_line":"                merger_gear.unRegisterFunction(f)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":"        self.create_branch(\u0027org/project\u0027, \u0027stable\u0027)"},{"line_number":561,"context_line":"        self.fake_gerrit.addEvent("}],"source_content_type":"text/x-python","patch_set":5,"id":"a00007a5_fc538d5e","side":"PARENT","line":558,"in_reply_to":"afb7815b_46e9ef51","updated":"2021-07-30 05:55:39.000000000","message":"I think I removed the second merger (line 553) by accident. My intention was to remove only the part after that where the gear functions are unregistered (merger_gear.unRegisterFunction()) since that shouldn\u0027t be necessary anymore with the merges via ZooKeeper.","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"}],"zuul/driver/git/gitconnection.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                setattr(change, attr, getattr(event, attr))"},{"line_number":114,"context_line":"            change.url \u003d \"\""},{"line_number":115,"context_line":"            change.files \u003d self.getChangeFilesUpdated("},{"line_number":116,"context_line":"                event.project_name, change.branch, event.oldrev)"},{"line_number":117,"context_line":"            self._change_cache.setdefault(branch, {})[event.newrev] \u003d change"},{"line_number":118,"context_line":"        elif event.ref:"},{"line_number":119,"context_line":"            # catch-all ref (ie, not a branch or head)"}],"source_content_type":"text/x-python","patch_set":13,"id":"05d5984c_581a3491","line":116,"updated":"2021-08-05 22:26:08.000000000","message":"This calls the method on line 67 of this file.  We do use needs_results above.  Basically, a merge request has 2 modes of operation:\n\n* needs_results\u003dFalse: the typical asynchronous request to merge a change in a pipeline and produce the results so zuul can proceed with a build.  The result of the merge will be placed in the pipeline results queue.\n* needs_results\u003dTrue: either the git driver or the configloader performing a synchronous merge request to find out something about a repo immediately.  In that case, there is no pipeline results queue for the request; instead the result will be placed in ZK and the merger api will return a future object which will contain the result after wait() completes.\n\nHaving said all that, we could probably infer needs_results from whether a buildset was provided with the requests, but this seems good for now.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":113,"context_line":"                setattr(change, attr, getattr(event, attr))"},{"line_number":114,"context_line":"            change.url \u003d \"\""},{"line_number":115,"context_line":"            change.files \u003d self.getChangeFilesUpdated("},{"line_number":116,"context_line":"                event.project_name, change.branch, event.oldrev)"},{"line_number":117,"context_line":"            self._change_cache.setdefault(branch, {})[event.newrev] \u003d change"},{"line_number":118,"context_line":"        elif event.ref:"},{"line_number":119,"context_line":"            # catch-all ref (ie, not a branch or head)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ac6938eb_106dffd4","line":116,"updated":"2021-08-05 18:54:35.000000000","message":"This is where we call getChangeFilesUpdated and we don\u0027t pass in needs_results. We also don\u0027t use needs_results above. Was the change above initially necessary and now no longer used?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3b6ac25ca3e670fccbca88bbab20a7676fc47558","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                setattr(change, attr, getattr(event, attr))"},{"line_number":114,"context_line":"            change.url \u003d \"\""},{"line_number":115,"context_line":"            change.files \u003d self.getChangeFilesUpdated("},{"line_number":116,"context_line":"                event.project_name, change.branch, event.oldrev)"},{"line_number":117,"context_line":"            self._change_cache.setdefault(branch, {})[event.newrev] \u003d change"},{"line_number":118,"context_line":"        elif event.ref:"},{"line_number":119,"context_line":"            # catch-all ref (ie, not a branch or head)"}],"source_content_type":"text/x-python","patch_set":13,"id":"2aeb843c_3b1580ce","line":116,"in_reply_to":"05d5984c_581a3491","updated":"2021-08-05 22:54:02.000000000","message":"I don\u0027t know why but I read needs_result as an argument to getChangeFilesUpdated and not to getFilesChanges. Sorry for the confusion here.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"}],"zuul/merger/client.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"917d0915bfb64be9238e789415d0ec06b0390151","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            zuul_event_id \u003d event.zuul_event_id"},{"line_number":48,"context_line":"        else:"},{"line_number":49,"context_line":"            zuul_event_id \u003d None"},{"line_number":50,"context_line":"        data[\"zuul_event_id\"] \u003d zuul_event_id"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        # We need the tenant, pipeline and queue names to put the merge result"},{"line_number":53,"context_line":"        # in the correct queue. The only source for those values in this"}],"source_content_type":"text/x-python","patch_set":2,"id":"db2f1936_76002313","line":50,"updated":"2021-07-26 20:48:53.000000000","message":"This mutates a supplied argument.  I don\u0027t think we should do that.  Why not add this as an extra parameter to the merge request rather than updating the data?","commit_id":"1d07501bb65bd9dba11eef7310cac15a20efd79d"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"f1f225404cd326adeff6eb811977f541a42de76a","unresolved":false,"context_lines":[{"line_number":133,"context_line":"            try:"},{"line_number":134,"context_line":"                self.cleanupLostMergeRequest(merge_request)"},{"line_number":135,"context_line":"            except Exception:"},{"line_number":136,"context_line":"                self.log.exception(\"Exception cleaning up lost merge request:\")"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def cleanupLostMergeRequest(self, merge_request):"},{"line_number":139,"context_line":"        merge_request.state \u003d MergeRequest.COMPLETED"}],"source_content_type":"text/x-python","patch_set":5,"id":"b23a36fb_5235e45f","line":136,"updated":"2021-07-31 20:00:26.000000000","message":"We should probably clean up the results too.\n\nAlso, do the tests exercise this?  I\u0027m not sure they do.  Maybe we can move it into the executor api and call it from test_zk.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    def mergeChanges(self, items, build_set, files\u003dNone, dirs\u003dNone,"},{"line_number":80,"context_line":"                     repo_state\u003dNone, precedence\u003dPRECEDENCE_NORMAL,"},{"line_number":81,"context_line":"                     branches\u003dNone, event\u003dNone):"},{"line_number":82,"context_line":"        data \u003d dict(items\u003ditems,"},{"line_number":83,"context_line":"                    files\u003dfiles,"},{"line_number":84,"context_line":"                    dirs\u003ddirs,"}],"source_content_type":"text/x-python","patch_set":13,"id":"9644384f_484d4eda","line":81,"updated":"2021-08-05 18:54:35.000000000","message":"This might be a good place to be clear when we pass an event or when we pass an event_id. The old code seemed to get an event here which it then passed the event_id of to submitJob(). But now we seem to pass the event the whole way through?\n\nIs that ok for the annotated logger? will it handle both types?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    def mergeChanges(self, items, build_set, files\u003dNone, dirs\u003dNone,"},{"line_number":80,"context_line":"                     repo_state\u003dNone, precedence\u003dPRECEDENCE_NORMAL,"},{"line_number":81,"context_line":"                     branches\u003dNone, event\u003dNone):"},{"line_number":82,"context_line":"        data \u003d dict(items\u003ditems,"},{"line_number":83,"context_line":"                    files\u003dfiles,"},{"line_number":84,"context_line":"                    dirs\u003ddirs,"}],"source_content_type":"text/x-python","patch_set":13,"id":"88318168_64cd778b","line":81,"updated":"2021-08-05 20:42:11.000000000","message":"We\u0027re not changing what\u0027s passed to submitJob; instead we\u0027re removing zuul_event_id from the payload for the job.\n\nI think what Felix is doing here is streamlining the code by putting the event id outside of the job parameters (but still it will be included in the request data).  Then if you check the merger server update in this change, it pulls the event id from the merge request instead of the payload.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    def mergeChanges(self, items, build_set, files\u003dNone, dirs\u003dNone,"},{"line_number":80,"context_line":"                     repo_state\u003dNone, precedence\u003dPRECEDENCE_NORMAL,"},{"line_number":81,"context_line":"                     branches\u003dNone, event\u003dNone):"},{"line_number":82,"context_line":"        data \u003d dict(items\u003ditems,"},{"line_number":83,"context_line":"                    files\u003dfiles,"},{"line_number":84,"context_line":"                    dirs\u003ddirs,"}],"source_content_type":"text/x-python","patch_set":13,"id":"f7c3c900_e21c50e3","line":81,"in_reply_to":"88318168_64cd778b","updated":"2021-08-05 21:36:25.000000000","message":"Ah got it, I see now that the event.zuul_event_id was passed via data and then event was also passed. I agree this is better.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":151,"context_line":"            # state RUNNING but gets completed/unlocked before the"},{"line_number":152,"context_line":"            # is_locked() check. Since we use the znode version, the"},{"line_number":153,"context_line":"            # update will fail in this case and we can simply ignore"},{"line_number":154,"context_line":"            # the exception."},{"line_number":155,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"9c912abc_9fd865ee","line":154,"updated":"2021-08-05 18:54:35.000000000","message":"I\u0027m having a hard time understanding this comment. Above it is stated that this request is by definition unlocked. I don\u0027t understand where there would be an is_locked() check that can fail if it wasn\u0027t already unlocked?\n\nSeparately we seem to acquire a scheduler cleanup lock before calling this method which should mean no other cleanups are running at the same time. This means we can only race the mergers?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":151,"context_line":"            # state RUNNING but gets completed/unlocked before the"},{"line_number":152,"context_line":"            # is_locked() check. Since we use the znode version, the"},{"line_number":153,"context_line":"            # update will fail in this case and we can simply ignore"},{"line_number":154,"context_line":"            # the exception."},{"line_number":155,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"8e2e366a_16e18917","line":154,"updated":"2021-08-05 20:42:11.000000000","message":"We only call this function if the merge request is unlocked.\n\nBut a lost merge request is defined as \"a merge request in running state that is unlocked\".\n\nWe check the state first, then we check the lock.  The following sequence can produce the error (prefixed by thread id for clarity):\n\n* [1] merge job is locked and in running state\n* [2] findLostMergeRequests lists all the merge jobs\n* [2] it performs a get on our merge job [note 1]\n* [2] it observes the state is \"running\"\n* [1] the merger finishes and updates the merge job state \"completed\"\n* [1] the merger unlocks the merge job\n* [2] findLostMergeRequests checks the lock on our merge job and sees it unlocked\n* [2] findLostMergeRequests declares the job lost\n* [2] this method updates the merge job to set it to \"completed\", using the zstat from the previous get (note 1 above)\n* [2] the update produces a BadVersionError because the zstat doesn\u0027t contain the latest node version\n\nTherefore we know we should ignore this job.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":151,"context_line":"            # state RUNNING but gets completed/unlocked before the"},{"line_number":152,"context_line":"            # is_locked() check. Since we use the znode version, the"},{"line_number":153,"context_line":"            # update will fail in this case and we can simply ignore"},{"line_number":154,"context_line":"            # the exception."},{"line_number":155,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"155b9e8e_8381214a","line":154,"in_reply_to":"8e2e366a_16e18917","updated":"2021-08-05 21:36:25.000000000","message":"Got it this helps a bunch thanks.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"}],"zuul/merger/server.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"917d0915bfb64be9238e789415d0ec06b0390151","unresolved":false,"context_lines":[{"line_number":413,"context_line":"        self.merger_api.unlock(merge_request)"},{"line_number":414,"context_line":"        # TODO (felix): If we want to optimize ZK requests, we could only call"},{"line_number":415,"context_line":"        # the remove() here."},{"line_number":416,"context_line":"        self.merger_api.remove(merge_request)"},{"line_number":417,"context_line":""},{"line_number":418,"context_line":""},{"line_number":419,"context_line":"class MergeServer(BaseMergeServer):"}],"source_content_type":"text/x-python","patch_set":2,"id":"77d2c8a3_813a7849","line":416,"updated":"2021-07-26 20:48:53.000000000","message":"If we unlock before deleting, we definitely need to update the state, otherwise another merger could start processing this.\n\nHowever, we can delete before unlocking, which will avoid that case and be more efficient.\n\nThere could still be a case where the merger crashes after submitting the complete event and before deleting.  We can ignore that case (there will be a duplicate result) or we write the result, delete, and even unlock all within a transaction.","commit_id":"1d07501bb65bd9dba11eef7310cac15a20efd79d"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"108cc93b15805827fd7fa8dee44e29480876e361","unresolved":false,"context_lines":[{"line_number":187,"context_line":"    def unpause(self):"},{"line_number":188,"context_line":"        self.log.debug(\u0027Resuming merger worker\u0027)"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def runMergerWorker(self):"},{"line_number":191,"context_line":"        while self._merger_running:"},{"line_number":192,"context_line":"            self.merger_loop_wake_event.wait()"},{"line_number":193,"context_line":"            self.merger_loop_wake_event.clear()"}],"source_content_type":"text/x-python","patch_set":5,"id":"9b139e9e_ea6e0edc","line":190,"updated":"2021-07-29 02:58:29.000000000","message":"The name \"MergerWorker\" is a bit confusing.  Previously, the Merger was implemented as a gearman Worker, so that\u0027s why the \"worker\" was started by the \"merger\".\n\nBut now we really just have the main merger thread; there aren\u0027t two distinct components.  I think we can drop the word \"worker\" from ewherewhere it\u0027s used (or maybe in some cases, like self.merger_worker change it to self.merger_thread).","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"0bc713bd6d58a584962f4cc76233992498120e64","unresolved":false,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def runMergerWorker(self):"},{"line_number":191,"context_line":"        while self._merger_running:"},{"line_number":192,"context_line":"            self.merger_loop_wake_event.wait()"},{"line_number":193,"context_line":"            self.merger_loop_wake_event.clear()"},{"line_number":194,"context_line":"            for merge_request in self.merger_api.next():"},{"line_number":195,"context_line":"                if not self._merger_running:"}],"source_content_type":"text/x-python","patch_set":5,"id":"afae1abc_a37b332f","line":192,"updated":"2021-07-29 02:44:30.000000000","message":"Disregard this comment, the check after we enter the iterator is sufficient.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2ed52e1211c5ddc29bdce1efdb94bd3b07c892d7","unresolved":false,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def runMergerWorker(self):"},{"line_number":191,"context_line":"        while self._merger_running:"},{"line_number":192,"context_line":"            self.merger_loop_wake_event.wait()"},{"line_number":193,"context_line":"            self.merger_loop_wake_event.clear()"},{"line_number":194,"context_line":"            for merge_request in self.merger_api.next():"},{"line_number":195,"context_line":"                if not self._merger_running:"}],"source_content_type":"text/x-python","patch_set":5,"id":"b4f88d75_0a4d32e0","line":192,"updated":"2021-07-29 01:23:49.000000000","message":"We should check if we\u0027re running after the wait, otherwise we might pick up a job (if one arrives just after we are requested to stop).","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"108cc93b15805827fd7fa8dee44e29480876e361","unresolved":false,"context_lines":[{"line_number":194,"context_line":"            for merge_request in self.merger_api.next():"},{"line_number":195,"context_line":"                if not self._merger_running:"},{"line_number":196,"context_line":"                    break"},{"line_number":197,"context_line":"                self._runMergerWorker(merge_request)"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def _runMergerWorker(self, merge_request):"},{"line_number":200,"context_line":"        if not self.merger_api.lock(merge_request, blocking\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":5,"id":"f7efe20a_63cb5510","line":197,"updated":"2021-07-29 02:58:29.000000000","message":"There is no exception handling within the merger thread, so if any job raises an exception, the thread will exit.  Previously, the gearman worker handled that for us, but now we need to do it.\n\nBasically, any exception in self._runMergerWorker (which I suggest renaming to _runMergeJob) should still report completion (but also, failure).  And separately, if that report has an error, it should log it and continue.  The thread should never exit unless explicitly requested.\n\nIt\u0027s possible this is what\u0027s causing the test errors, but I still haven\u0027t caught it in logs.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"335fe43fc25ff5d65e9d094b88bf477ccb6c7b00","unresolved":false,"context_lines":[{"line_number":372,"context_line":"        self.merger_api.unlock(merge_request)"},{"line_number":373,"context_line":"        # TODO (felix): If we want to optimize ZK requests, we could only call"},{"line_number":374,"context_line":"        # the remove() here."},{"line_number":375,"context_line":"        self.merger_api.remove(merge_request)"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"class MergeServer(BaseMergeServer):"}],"source_content_type":"text/x-python","patch_set":5,"id":"2f3d2cdd_3ef7e0e7","line":375,"updated":"2021-07-29 02:19:23.000000000","message":"See comment on earlier PS.","commit_id":"406d739a88f57f5f609edc0e07f370872410053a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"af39fc7f0a894fdfa09512b79e066c726535300e","unresolved":false,"context_lines":[{"line_number":183,"context_line":"        self.log.debug(\u0027Pausing merger\u0027)"},{"line_number":184,"context_line":"        # TODO (felix): How to pause/unpause the merger? We could pause"},{"line_number":185,"context_line":"        # the Merger API as well, so any cache updates don\u0027t set the wake"},{"line_number":186,"context_line":"        # event. Not sure if that is a proper solution though."},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    def unpause(self):"},{"line_number":189,"context_line":"        self.log.debug(\u0027Resuming merger\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ff5aed4_cf1d6307","line":186,"updated":"2021-08-01 14:23:30.000000000","message":"I think the easiest thing to do is to just set an internal flag.  Below in runMerger, we would just \"continue\" after clearing the wake event if the pause flag is set.","commit_id":"b5155f21b805fdd0b81dd39e50b727c291438d85"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"cb5c1dcd632fc26cb17244e7e4d2868ec23b3e11","unresolved":false,"context_lines":[{"line_number":183,"context_line":"        self.log.debug(\u0027Pausing merger\u0027)"},{"line_number":184,"context_line":"        # TODO (felix): How to pause/unpause the merger? We could pause"},{"line_number":185,"context_line":"        # the Merger API as well, so any cache updates don\u0027t set the wake"},{"line_number":186,"context_line":"        # event. Not sure if that is a proper solution though."},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    def unpause(self):"},{"line_number":189,"context_line":"        self.log.debug(\u0027Resuming merger\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"98047367_5910499b","line":186,"updated":"2021-08-01 14:34:34.000000000","message":"I uploaded a followup to do that.","commit_id":"b5155f21b805fdd0b81dd39e50b727c291438d85"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"af39fc7f0a894fdfa09512b79e066c726535300e","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        self.merger_api.update(merge_request)"},{"line_number":214,"context_line":"        # TODO (felix): What about the executor server implementation"},{"line_number":215,"context_line":"        # when it\u0027s currently not accepting work? Do we have to ignore"},{"line_number":216,"context_line":"        # merge requests in that case as well?"},{"line_number":217,"context_line":"        self.log.debug(\"Next executed merge job: %s\", merge_request)"},{"line_number":218,"context_line":"        result \u003d None"},{"line_number":219,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"b68baae1_0aa44100","line":216,"updated":"2021-08-01 14:23:30.000000000","message":"No, the merger doesn\u0027t know anything about the executor.  The executor pauses its internal merger when it is paused (but it doesn\u0027t pause it when it stops accepting jobs).","commit_id":"b5155f21b805fdd0b81dd39e50b727c291438d85"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"b6162956fb6d004d86a2d23c4c9fe8f9a8f42346","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        self.merger_api.update(merge_request)"},{"line_number":214,"context_line":"        # TODO (felix): What about the executor server implementation"},{"line_number":215,"context_line":"        # when it\u0027s currently not accepting work? Do we have to ignore"},{"line_number":216,"context_line":"        # merge requests in that case as well?"},{"line_number":217,"context_line":"        self.log.debug(\"Next executed merge job: %s\", merge_request)"},{"line_number":218,"context_line":"        result \u003d None"},{"line_number":219,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"869d5ce5_ea700bb6","line":216,"in_reply_to":"b68baae1_0aa44100","updated":"2021-08-02 08:11:46.000000000","message":"Ok, so we can just remove this todo here","commit_id":"b5155f21b805fdd0b81dd39e50b727c291438d85"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    def start(self):"},{"line_number":149,"context_line":"        self.log.debug(\u0027Starting merger\u0027)"},{"line_number":150,"context_line":"        self._merger_running \u003d True"},{"line_number":151,"context_line":"        self.log.debug(\u0027Cleaning any stale git index.lock files\u0027)"},{"line_number":152,"context_line":"        for (dirpath, dirnames, filenames) in os.walk(self.merge_root):"},{"line_number":153,"context_line":"            if \u0027.git\u0027 in dirnames:"}],"source_content_type":"text/x-python","patch_set":13,"id":"f569be54_67f3501a","line":150,"updated":"2021-08-05 18:54:35.000000000","message":"Might consider setting this flag immediately before starting the thread below. This way there is less time where the thread is not running but the flag is set?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"b50bcce0_cc025b10","line":175,"updated":"2021-08-05 20:42:11.000000000","message":"Or remove this join in favor of the one below?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"9c019af94dd4d825de463f46a170f3e9e94f5ed0","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"10567ed1_cbe4682d","line":175,"updated":"2021-08-05 23:27:11.000000000","message":"Right, other places call it.\n\nTo shut down a merger:\n\nmerger.stop()\nmerger.join()\n\nSame is true for the executor.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"28beeafb_e3a1f6db","line":175,"updated":"2021-08-05 22:26:08.000000000","message":"Well, the reason we have stop and join separate is so that when we shut down, we can stop() everything and the join() everything; that way shutdown is parallel, not serial.  That\u0027s why I think we should drop this join and keep the one below.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":171,"context_line":"    def stop(self):"},{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"28603fac_0e0ecff5","line":175,"range":{"start_line":174,"start_character":0,"end_line":175,"end_character":33},"updated":"2021-08-05 18:54:35.000000000","message":"Why not replace this with self.join() or remove the join def below?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"84efb8833186b073bff271c3ff1266b2a28f1297","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"115fc0ef_899f1ad9","line":175,"in_reply_to":"10567ed1_cbe4682d","updated":"2021-08-05 23:29:18.000000000","message":"Got it.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3b6ac25ca3e670fccbca88bbab20a7676fc47558","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"02ad495e_4f6c998a","line":175,"in_reply_to":"28beeafb_e3a1f6db","updated":"2021-08-05 22:54:02.000000000","message":"I think I may be confused now. What join call below? We define a join method but MergeServer.stop() doesn\u0027t seem to call it?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":172,"context_line":"        self.log.debug(\u0027Stopping merger\u0027)"},{"line_number":173,"context_line":"        self._merger_running \u003d False"},{"line_number":174,"context_line":"        self.merger_loop_wake_event.set()"},{"line_number":175,"context_line":"        self.merger_thread.join()"},{"line_number":176,"context_line":"        self.merger_cleanup_election.cancel()"},{"line_number":177,"context_line":"        self.zk_client.disconnect()"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"2a4306f5_3cff5ecf","line":175,"in_reply_to":"b50bcce0_cc025b10","updated":"2021-08-05 21:36:25.000000000","message":"Yup basically we should call self.join() here I think.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _runMergeJob(self, merge_request):"},{"line_number":208,"context_line":"        if not self.merger_api.lock(merge_request, blocking\u003dFalse):"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"        merge_request.state \u003d MergeRequest.RUNNING"},{"line_number":212,"context_line":"        params \u003d self.merger_api.getMergeParams(merge_request)"}],"source_content_type":"text/x-python","patch_set":13,"id":"83f32098_08bd1def","line":209,"updated":"2021-08-05 18:54:35.000000000","message":"Is this worthy of logging? We will skip the merge request on the assumption something else has the lock and is handling it. Might be painful to debug lost merges if we don\u0027t record this?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _runMergeJob(self, merge_request):"},{"line_number":208,"context_line":"        if not self.merger_api.lock(merge_request, blocking\u003dFalse):"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"        merge_request.state \u003d MergeRequest.RUNNING"},{"line_number":212,"context_line":"        params \u003d self.merger_api.getMergeParams(merge_request)"}],"source_content_type":"text/x-python","patch_set":13,"id":"c7d6ba9d_197b9932","line":209,"updated":"2021-08-05 20:42:11.000000000","message":"We probably could; it would be a little verbose.  But this is the same behavior in the executor and we haven\u0027t needed it there yet.  I think I\u0027d prefer to wait and see if we need to add it.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _runMergeJob(self, merge_request):"},{"line_number":208,"context_line":"        if not self.merger_api.lock(merge_request, blocking\u003dFalse):"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"        merge_request.state \u003d MergeRequest.RUNNING"},{"line_number":212,"context_line":"        params \u003d self.merger_api.getMergeParams(merge_request)"}],"source_content_type":"text/x-python","patch_set":13,"id":"099735d1_9449b5d5","line":209,"in_reply_to":"c7d6ba9d_197b9932","updated":"2021-08-05 21:36:25.000000000","message":"Ok","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        self.merger_api.clearMergeParams(merge_request)"},{"line_number":214,"context_line":"        # Directly update the merge request in ZooKeeper, so we don\u0027t loop over"},{"line_number":215,"context_line":"        # and try to lock it again and again."},{"line_number":216,"context_line":"        self.merger_api.update(merge_request)"},{"line_number":217,"context_line":"        self.log.debug(\"Next executed merge job: %s\", merge_request)"},{"line_number":218,"context_line":"        result \u003d None"},{"line_number":219,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"0e49ebcc_764b8292","line":216,"updated":"2021-08-05 20:42:11.000000000","message":"I agree.  It is worth noting that if the ZK connection disappears, so will our lock, so we don\u0027t need to worry about that.  But if there\u0027s some sort of error that we accidentally don\u0027t handle lower in the call stack then we could leak the lock.  So better safe than sorry.\n\nThe executor actually handles this by adding the unlock in the equivalent of the runMerger method.  We should do that here.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"        merge_request.state \u003d MergeRequest.RUNNING"},{"line_number":212,"context_line":"        params \u003d self.merger_api.getMergeParams(merge_request)"},{"line_number":213,"context_line":"        self.merger_api.clearMergeParams(merge_request)"},{"line_number":214,"context_line":"        # Directly update the merge request in ZooKeeper, so we don\u0027t loop over"},{"line_number":215,"context_line":"        # and try to lock it again and again."},{"line_number":216,"context_line":"        self.merger_api.update(merge_request)"},{"line_number":217,"context_line":"        self.log.debug(\"Next executed merge job: %s\", merge_request)"},{"line_number":218,"context_line":"        result \u003d None"},{"line_number":219,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"1957d18f_74a3dea3","line":216,"range":{"start_line":212,"start_character":8,"end_line":216,"end_character":45},"updated":"2021-08-05 18:54:35.000000000","message":"I think these calls can raise as they interact with zookeeper. If they raise we will not unlock the lock we grabbed above.\n\nPerhaps we should run everything after the lock acquisition in the try block?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"84efb8833186b073bff271c3ff1266b2a28f1297","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        self.merger_api.clearMergeParams(merge_request)"},{"line_number":214,"context_line":"        # Directly update the merge request in ZooKeeper, so we don\u0027t loop over"},{"line_number":215,"context_line":"        # and try to lock it again and again."},{"line_number":216,"context_line":"        self.merger_api.update(merge_request)"},{"line_number":217,"context_line":"        self.log.debug(\"Next executed merge job: %s\", merge_request)"},{"line_number":218,"context_line":"        result \u003d None"},{"line_number":219,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":13,"id":"c7583f40_0678d1b1","line":216,"in_reply_to":"0e49ebcc_764b8292","updated":"2021-08-05 23:29:18.000000000","message":"This was corrected in ps14","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":222,"context_line":"            self.log.exception(\"Error running merge job:\")"},{"line_number":223,"context_line":"        finally:"},{"line_number":224,"context_line":"            try:"},{"line_number":225,"context_line":"                self.completeMergeJob(merge_request, result)"},{"line_number":226,"context_line":"            except Exception:"},{"line_number":227,"context_line":"                self.log.exception(\"Error completing merge job:\")"},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"998403ec_6e12a421","line":225,"updated":"2021-08-05 18:54:35.000000000","message":"I have a slight preference for maintaining symmetry with the lock and unlock calls if we can. I know completeMergeJob does the unlock but moving that here would make it clearer to see we are unlocking.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":222,"context_line":"            self.log.exception(\"Error running merge job:\")"},{"line_number":223,"context_line":"        finally:"},{"line_number":224,"context_line":"            try:"},{"line_number":225,"context_line":"                self.completeMergeJob(merge_request, result)"},{"line_number":226,"context_line":"            except Exception:"},{"line_number":227,"context_line":"                self.log.exception(\"Error completing merge job:\")"},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"825ec72a_172d4530","line":225,"updated":"2021-08-05 20:42:11.000000000","message":"I think if we structure this like we do the executor, it will be robust and easy to follow.  It won\u0027t be exactly symmetrical here, but I think we can handle it.\n\nI\u0027d at least like this change to be structured like the executor, and then if we don\u0027t like that, maybe we can change both simultaneously in a followup?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":222,"context_line":"            self.log.exception(\"Error running merge job:\")"},{"line_number":223,"context_line":"        finally:"},{"line_number":224,"context_line":"            try:"},{"line_number":225,"context_line":"                self.completeMergeJob(merge_request, result)"},{"line_number":226,"context_line":"            except Exception:"},{"line_number":227,"context_line":"                self.log.exception(\"Error completing merge job:\")"},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"e5a87458_8de4e5cc","line":225,"in_reply_to":"825ec72a_172d4530","updated":"2021-08-05 21:36:25.000000000","message":"Keeping the two in sync makes sense to me. We can update in followups if we have a preference change.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":322,"context_line":""},{"line_number":323,"context_line":"        if result is not None:"},{"line_number":324,"context_line":"            payload \u003d json.dumps(result)"},{"line_number":325,"context_line":"            self.log.debug(\"Completed %s job %s: payload size: %s\","},{"line_number":326,"context_line":"                           merge_request.job_type, merge_request.uuid,"},{"line_number":327,"context_line":"                           sys.getsizeof(payload))"},{"line_number":328,"context_line":"            merged \u003d result.get(\"merged\", False)"}],"source_content_type":"text/x-python","patch_set":13,"id":"34ecf23e_02e87e0c","line":325,"range":{"start_line":325,"start_character":49,"end_line":325,"end_character":66},"updated":"2021-08-05 18:54:35.000000000","message":"Does the length of the serialized json object tell us something that None or not None wouldn\u0027t?","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"92bf6973ebc523cf4badba6091386415a8be490a","unresolved":false,"context_lines":[{"line_number":322,"context_line":""},{"line_number":323,"context_line":"        if result is not None:"},{"line_number":324,"context_line":"            payload \u003d json.dumps(result)"},{"line_number":325,"context_line":"            self.log.debug(\"Completed %s job %s: payload size: %s\","},{"line_number":326,"context_line":"                           merge_request.job_type, merge_request.uuid,"},{"line_number":327,"context_line":"                           sys.getsizeof(payload))"},{"line_number":328,"context_line":"            merged \u003d result.get(\"merged\", False)"}],"source_content_type":"text/x-python","patch_set":13,"id":"4f176ece_dfbfc194","line":325,"updated":"2021-08-05 22:26:08.000000000","message":"Yep.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":322,"context_line":""},{"line_number":323,"context_line":"        if result is not None:"},{"line_number":324,"context_line":"            payload \u003d json.dumps(result)"},{"line_number":325,"context_line":"            self.log.debug(\"Completed %s job %s: payload size: %s\","},{"line_number":326,"context_line":"                           merge_request.job_type, merge_request.uuid,"},{"line_number":327,"context_line":"                           sys.getsizeof(payload))"},{"line_number":328,"context_line":"            merged \u003d result.get(\"merged\", False)"}],"source_content_type":"text/x-python","patch_set":13,"id":"532327e2_0b846b39","line":325,"updated":"2021-08-05 20:42:11.000000000","message":"Yes, the objective of this log line was to track down the problem caused by writing packets that were too large to ZK.  (The issue we just fixed with the sharding changes.)\n\nMaybe we don\u0027t need it now, but this change isn\u0027t changing that, it\u0027s just retaining the existing logging.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"db25753dd579ec253c94db86478a97f258b29623","unresolved":false,"context_lines":[{"line_number":322,"context_line":""},{"line_number":323,"context_line":"        if result is not None:"},{"line_number":324,"context_line":"            payload \u003d json.dumps(result)"},{"line_number":325,"context_line":"            self.log.debug(\"Completed %s job %s: payload size: %s\","},{"line_number":326,"context_line":"                           merge_request.job_type, merge_request.uuid,"},{"line_number":327,"context_line":"                           sys.getsizeof(payload))"},{"line_number":328,"context_line":"            merged \u003d result.get(\"merged\", False)"}],"source_content_type":"text/x-python","patch_set":13,"id":"9136c6bb_200b3ac2","line":325,"in_reply_to":"532327e2_0b846b39","updated":"2021-08-05 21:36:25.000000000","message":"Got it. And I agree this isn\u0027t changing the old log behavior but now we check against None above. I thought maybe we were looking for 0 vs not 0 length but we are looking for small vs large instead.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":332,"context_line":"            item_in_branches \u003d result.get(\"item_in_branches\", [])"},{"line_number":333,"context_line":"            files \u003d result.get(\"files\", {})"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"            log.info("},{"line_number":336,"context_line":"                \"Merge %s complete, merged: %s, updated: %s, commit: %s, \""},{"line_number":337,"context_line":"                \"branches: %s\","},{"line_number":338,"context_line":"                merge_request,"}],"source_content_type":"text/x-python","patch_set":13,"id":"203f7d21_2a905ffa","line":335,"updated":"2021-08-05 20:42:11.000000000","message":"Different data; different level.  I\u0027m happy to revisit these, but this log message is also just retained in this change, this isn\u0027t a change.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":332,"context_line":"            item_in_branches \u003d result.get(\"item_in_branches\", [])"},{"line_number":333,"context_line":"            files \u003d result.get(\"files\", {})"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"            log.info("},{"line_number":336,"context_line":"                \"Merge %s complete, merged: %s, updated: %s, commit: %s, \""},{"line_number":337,"context_line":"                \"branches: %s\","},{"line_number":338,"context_line":"                merge_request,"}],"source_content_type":"text/x-python","patch_set":13,"id":"4f4ed5c5_8146c0ae","line":335,"updated":"2021-08-05 18:54:35.000000000","message":"Seems like this log message provides far more info than the one above.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3b6ac25ca3e670fccbca88bbab20a7676fc47558","unresolved":true,"context_lines":[{"line_number":205,"context_line":"                            self.log, merge_request.event_id"},{"line_number":206,"context_line":"                        )"},{"line_number":207,"context_line":"                        log.exception(\"Exception while performing merge\")"},{"line_number":208,"context_line":"                        self.completeMergeJob(merge_request, None)"},{"line_number":209,"context_line":"            except Exception:"},{"line_number":210,"context_line":"                self.log.exception(\"Error in merge thread:\")"},{"line_number":211,"context_line":"                time.sleep(5)"}],"source_content_type":"text/x-python","patch_set":14,"id":"747743ae_089c009d","line":208,"updated":"2021-08-05 22:54:02.000000000","message":"Can we move this into a finally block and drop the finally block call in _runMergeJob()? Then we aren\u0027t tracking two places we can call completeMergeJob() through.","commit_id":"abb89099ce2a5fa072d6e649863e69ef605c6aec"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e410a5aeeef1d53154d513d4a96a0263c3aa1300","unresolved":false,"context_lines":[{"line_number":205,"context_line":"                            self.log, merge_request.event_id"},{"line_number":206,"context_line":"                        )"},{"line_number":207,"context_line":"                        log.exception(\"Exception while performing merge\")"},{"line_number":208,"context_line":"                        self.completeMergeJob(merge_request, None)"},{"line_number":209,"context_line":"            except Exception:"},{"line_number":210,"context_line":"                self.log.exception(\"Error in merge thread:\")"},{"line_number":211,"context_line":"                time.sleep(5)"}],"source_content_type":"text/x-python","patch_set":14,"id":"6d9468fd_6c16569a","line":208,"updated":"2021-08-05 23:27:07.000000000","message":"We could, but it\u0027s not straightforward -- this is exactly equivalent to what we already did in the executor, and I\u0027d like to keep them in sync.","commit_id":"abb89099ce2a5fa072d6e649863e69ef605c6aec"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"84efb8833186b073bff271c3ff1266b2a28f1297","unresolved":false,"context_lines":[{"line_number":205,"context_line":"                            self.log, merge_request.event_id"},{"line_number":206,"context_line":"                        )"},{"line_number":207,"context_line":"                        log.exception(\"Exception while performing merge\")"},{"line_number":208,"context_line":"                        self.completeMergeJob(merge_request, None)"},{"line_number":209,"context_line":"            except Exception:"},{"line_number":210,"context_line":"                self.log.exception(\"Error in merge thread:\")"},{"line_number":211,"context_line":"                time.sleep(5)"}],"source_content_type":"text/x-python","patch_set":14,"id":"d70a9a67_5393caeb","line":208,"in_reply_to":"6d9468fd_6c16569a","updated":"2021-08-05 23:29:18.000000000","message":"Ok","commit_id":"abb89099ce2a5fa072d6e649863e69ef605c6aec"},{"author":{"_account_id":16068,"name":"Tobias Henkel","email":"tobias.henkel@bmw.de","username":"tobias.henkel"},"change_message_id":"cbb0d188ab59c2a6ae67a3a0fe68b70927e2dfb9","unresolved":true,"context_lines":[{"line_number":372,"context_line":"                        updated,"},{"line_number":373,"context_line":"                        commit,"},{"line_number":374,"context_line":"                        files,"},{"line_number":375,"context_line":"                        repo_state,"},{"line_number":376,"context_line":"                        item_in_branches,"},{"line_number":377,"context_line":"                    )"},{"line_number":378,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"31269392_5d6abdf6","line":375,"updated":"2021-08-06 19:13:26.000000000","message":"When we have the pipeline state in zk I think it makes sense to put the repo state directly there instead of forwarding it through multiple copies via events.","commit_id":"c4129036a6802a776c4a2928bcd75611d188b2dd"}],"zuul/scheduler.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"6c748c8d9a8f4bd5aa07e482ae0d24611255b1dc","unresolved":false,"context_lines":[{"line_number":1343,"context_line":"        self.log.debug(\"Checking if all builds are complete\")"},{"line_number":1344,"context_line":"        if self.merger.areMergesOutstanding():"},{"line_number":1345,"context_line":"            self.log.debug(\"Waiting on merger\")"},{"line_number":1346,"context_line":"            return False"},{"line_number":1347,"context_line":"        waiting \u003d False"},{"line_number":1348,"context_line":"        for tenant in self.abide.tenants.values():"},{"line_number":1349,"context_line":"            for pipeline in tenant.layout.pipelines.values():"}],"source_content_type":"text/x-python","patch_set":5,"id":"aa29c3b3_e2be59bd","side":"PARENT","line":1346,"updated":"2021-08-02 14:11:36.000000000","message":"Okay, the only place this is used is a test which doesn\u0027t involve any merge jobs, so this is probably okay.\n\n(In another change, we can probably remove the whole thing and update the test to use the typical test methods.)","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"335fe43fc25ff5d65e9d094b88bf477ccb6c7b00","unresolved":false,"context_lines":[{"line_number":1343,"context_line":"        self.log.debug(\"Checking if all builds are complete\")"},{"line_number":1344,"context_line":"        if self.merger.areMergesOutstanding():"},{"line_number":1345,"context_line":"            self.log.debug(\"Waiting on merger\")"},{"line_number":1346,"context_line":"            return False"},{"line_number":1347,"context_line":"        waiting \u003d False"},{"line_number":1348,"context_line":"        for tenant in self.abide.tenants.values():"},{"line_number":1349,"context_line":"            for pipeline in tenant.layout.pipelines.values():"}],"source_content_type":"text/x-python","patch_set":5,"id":"71a83d25_7de96c71","side":"PARENT","line":1346,"updated":"2021-07-29 02:19:23.000000000","message":"What\u0027s the reason for dropping this?","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"b6162956fb6d004d86a2d23c4c9fe8f9a8f42346","unresolved":false,"context_lines":[{"line_number":1343,"context_line":"        self.log.debug(\"Checking if all builds are complete\")"},{"line_number":1344,"context_line":"        if self.merger.areMergesOutstanding():"},{"line_number":1345,"context_line":"            self.log.debug(\"Waiting on merger\")"},{"line_number":1346,"context_line":"            return False"},{"line_number":1347,"context_line":"        waiting \u003d False"},{"line_number":1348,"context_line":"        for tenant in self.abide.tenants.values():"},{"line_number":1349,"context_line":"            for pipeline in tenant.layout.pipelines.values():"}],"source_content_type":"text/x-python","patch_set":5,"id":"2e7a301a_b2eb935a","side":"PARENT","line":1346,"in_reply_to":"71a83d25_7de96c71","updated":"2021-08-02 08:11:46.000000000","message":"I dropped the self.jobs list in the merger client as I thought that this is not needed anymore. This method is not used in the \"production code\", and in the tests we could always retrieve the outstanding/queued jobs via the central merger api in the ZuulTestCase class (e.g. what\u0027s currently done in the __areAllMergeJobsWaiting() function in tests/base.py.","commit_id":"6cdfe679f64faa1c2f9e0da9eefac623f742f263"}],"zuul/zk/merger.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9dcc742a70ee3fe4bf73567c0c15865bfe93b82","unresolved":false,"context_lines":[{"line_number":493,"context_line":"                return None"},{"line_number":494,"context_line":"            return self._bytesToDict(data)"},{"line_number":495,"context_line":""},{"line_number":496,"context_line":"    def deleteMergeResult(self, path):"},{"line_number":497,"context_line":"        with suppress(NoNodeError):"},{"line_number":498,"context_line":"            self.kazoo_client.delete(path, recursive\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5ca58980_232d7621","line":496,"updated":"2021-08-05 20:42:11.000000000","message":"Agreed.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11a3b0e118439478abd59850e4fbe2609b1881b0","unresolved":true,"context_lines":[{"line_number":493,"context_line":"                return None"},{"line_number":494,"context_line":"            return self._bytesToDict(data)"},{"line_number":495,"context_line":""},{"line_number":496,"context_line":"    def deleteMergeResult(self, path):"},{"line_number":497,"context_line":"        with suppress(NoNodeError):"},{"line_number":498,"context_line":"            self.kazoo_client.delete(path, recursive\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":13,"id":"1196f876_701d1ae8","line":496,"updated":"2021-08-05 18:54:35.000000000","message":"This method appears to be unused.","commit_id":"648de59cd4d60b830f48b2d02bab1f553cb6111b"}]}