)]}'
{"tests/base.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c4075cf69e2e5ab4d191055bba9d35466256aeae","unresolved":false,"context_lines":[{"line_number":4878,"context_line":"                    log \u003d get_annotated_logger("},{"line_number":4879,"context_line":"                        self.log, event\u003dNone, build\u003dbuild_uuid"},{"line_number":4880,"context_line":"                    )"},{"line_number":4881,"context_line":"                    log.debug(\"%s is not finalized\", build_uuid)"},{"line_number":4882,"context_line":"                    return False"},{"line_number":4883,"context_line":"        return True"},{"line_number":4884,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"76ea216c_49624929","line":4881,"updated":"2021-04-12 21:39:32.000000000","message":"Nit: we could write this as \"Build is not finalized\" since the uuid is already in the message.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"738c66029bde7a77e53ee57c9980be9400af2372","unresolved":false,"context_lines":[{"line_number":4878,"context_line":"                    log \u003d get_annotated_logger("},{"line_number":4879,"context_line":"                        self.log, event\u003dNone, build\u003dbuild_uuid"},{"line_number":4880,"context_line":"                    )"},{"line_number":4881,"context_line":"                    log.debug(\"%s is not finalized\", build_uuid)"},{"line_number":4882,"context_line":"                    return False"},{"line_number":4883,"context_line":"        return True"},{"line_number":4884,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"a7b4a9cd_46fa6e3e","line":4881,"in_reply_to":"76ea216c_49624929","updated":"2021-04-15 14:24:08.000000000","message":"will do","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"}],"tests/unit/test_github_driver.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"d6ca9a84a567a5116128cf95b512e62abcf10f7c","unresolved":true,"context_lines":[{"line_number":352,"context_line":"        # below don\u0027t race against more jobs being queued."},{"line_number":353,"context_line":"        self.commitConfigUpdate(\u0027org/common-config\u0027,"},{"line_number":354,"context_line":"                                \u0027layouts/no-timer-github.yaml\u0027)"},{"line_number":355,"context_line":"        self.scheds.execute(lambda app: app.sched.reconfigure(app.config))"},{"line_number":356,"context_line":"        self.waitUntilSettled()"},{"line_number":357,"context_line":"        # If APScheduler is in mid-event when we remove the job, we"},{"line_number":358,"context_line":"        # can end up with one more event firing, so give it an extra"}],"source_content_type":"text/x-python","patch_set":7,"id":"61e45431_49e3f26d","line":355,"updated":"2021-04-26 22:55:17.000000000","message":"Does the commitConfigUpdate() above need to run before we release builds to ensure there isn\u0027t a race between timer jobs running and this update happening?","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5d4ac84fe9b10ecc9840667944f8feb28f0099e9","unresolved":false,"context_lines":[{"line_number":352,"context_line":"        # below don\u0027t race against more jobs being queued."},{"line_number":353,"context_line":"        self.commitConfigUpdate(\u0027org/common-config\u0027,"},{"line_number":354,"context_line":"                                \u0027layouts/no-timer-github.yaml\u0027)"},{"line_number":355,"context_line":"        self.scheds.execute(lambda app: app.sched.reconfigure(app.config))"},{"line_number":356,"context_line":"        self.waitUntilSettled()"},{"line_number":357,"context_line":"        # If APScheduler is in mid-event when we remove the job, we"},{"line_number":358,"context_line":"        # can end up with one more event firing, so give it an extra"}],"source_content_type":"text/x-python","patch_set":7,"id":"c783ad9c_72730b58","line":355,"updated":"2021-04-27 01:40:14.000000000","message":"This seems to be failing now and isn\u0027t related to the change.  I think we should back it out.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"}],"zuul/executor/client.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c4075cf69e2e5ab4d191055bba9d35466256aeae","unresolved":false,"context_lines":[{"line_number":189,"context_line":"            completed_event \u003d BuildCompletedEvent(build.uuid, result)"},{"line_number":190,"context_line":"            self.result_events[pipeline.tenant.name][pipeline.name].put("},{"line_number":191,"context_line":"                completed_event"},{"line_number":192,"context_line":"            )"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"            return build"},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"c65b1323_4b928405","line":192,"updated":"2021-04-12 21:39:32.000000000","message":"Review note: the lifecycle of the items in self.builds is changed a bit here.  Noop jobs now get an entry in self.builds, but for all jobs (noop and otherwise) the entry should be removed in the scheduler now, either upon normal completion, or in the new cleanup methods.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"738c66029bde7a77e53ee57c9980be9400af2372","unresolved":false,"context_lines":[{"line_number":189,"context_line":"            completed_event \u003d BuildCompletedEvent(build.uuid, result)"},{"line_number":190,"context_line":"            self.result_events[pipeline.tenant.name][pipeline.name].put("},{"line_number":191,"context_line":"                completed_event"},{"line_number":192,"context_line":"            )"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"            return build"},{"line_number":195,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bc2aeab8_0f179f70","line":192,"in_reply_to":"c65b1323_4b928405","updated":"2021-04-15 14:24:08.000000000","message":"That\u0027s right. I will add a section about this to the commit message. AFAIK you don\u0027t like to put comments about such \"changed behaviour\" in the code as this might confuse people which aren\u0027t aware of the old behaviour 😊","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"d6ca9a84a567a5116128cf95b512e62abcf10f7c","unresolved":true,"context_lines":[{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    def onWorkStatus(self, job):"},{"line_number":306,"context_line":"        data \u003d getJobData(job)"},{"line_number":307,"context_line":"        self.log.debug(\"Build %s update %s\" % (job, data))"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"    def onDisconnect(self, job):"},{"line_number":310,"context_line":"        self.log.info(\"Gearman job %s lost due to disconnect\" % job)"}],"source_content_type":"text/x-python","patch_set":7,"id":"cbe4e183_94fd5911","line":307,"updated":"2021-04-26 22:55:17.000000000","message":"This method only exists to log the job data at this point. Would it be better to simply pass it? Or remove it altogether?\n\nAs an alternative should we keep the method largely as is? its purpose is to return info back to gearman about build status. We can clean this up when gearman goes away? I think for now it would only make sense for the merge jobs?","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"80da4bd3dc66535f2105e6f1615a8c18e0a21531","unresolved":true,"context_lines":[{"line_number":304,"context_line":""},{"line_number":305,"context_line":"    def onWorkStatus(self, job):"},{"line_number":306,"context_line":"        data \u003d getJobData(job)"},{"line_number":307,"context_line":"        self.log.debug(\"Build %s update %s\" % (job, data))"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"    def onDisconnect(self, job):"},{"line_number":310,"context_line":"        self.log.info(\"Gearman job %s lost due to disconnect\" % job)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ef0a28bc_b332ce05","line":307,"in_reply_to":"cbe4e183_94fd5911","updated":"2021-04-27 05:13:49.000000000","message":"IIRC I remove this method in the follow up change, when we switch from Gearman to ZooKeeper for the actual job execution (BuildRequest). In here I just moved all the different parts that deal with build result events into a single place in the scheduler.\n\nI think we should keep it for now, to not cause any issues with gearman.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"}],"zuul/executor/server.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c4075cf69e2e5ab4d191055bba9d35466256aeae","unresolved":false,"context_lines":[{"line_number":1169,"context_line":""},{"line_number":1170,"context_line":"        self.executor_server.updateBuildStatus(self.job, data)"},{"line_number":1171,"context_line":"        # TODO (felix): Is this needed at all?"},{"line_number":1172,"context_line":"        self.job.sendWorkStatus(0, 100)"},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"        result \u003d self.runPlaybooks(args)"},{"line_number":1175,"context_line":"        success \u003d result \u003d\u003d \u0027SUCCESS\u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"6cf9c641_2d36f8c4","line":1172,"updated":"2021-04-12 21:39:32.000000000","message":"I don\u0027t believe it\u0027s currently used.  It could be used to indicate the progress, but we do that in the scheduler with the time database instead.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"738c66029bde7a77e53ee57c9980be9400af2372","unresolved":false,"context_lines":[{"line_number":1169,"context_line":""},{"line_number":1170,"context_line":"        self.executor_server.updateBuildStatus(self.job, data)"},{"line_number":1171,"context_line":"        # TODO (felix): Is this needed at all?"},{"line_number":1172,"context_line":"        self.job.sendWorkStatus(0, 100)"},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"        result \u003d self.runPlaybooks(args)"},{"line_number":1175,"context_line":"        success \u003d result \u003d\u003d \u0027SUCCESS\u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"3b947856_87b2d3af","line":1172,"in_reply_to":"6cf9c641_2d36f8c4","updated":"2021-04-15 14:24:08.000000000","message":"Ok, then I will simply remove it","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"}],"zuul/scheduler.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"bd2a0cb6322f773e3125c3e8889857e0d673b3ca","unresolved":false,"context_lines":[{"line_number":928,"context_line":"        # TODO (felix): The cancel call can be removed once we switch the"},{"line_number":929,"context_line":"        # builds to ZooKeeper. It\u0027s main purpose is to cancel/complete the"},{"line_number":930,"context_line":"        # runnig gearman job which otherwise breaks the tests in case of a"},{"line_number":931,"context_line":"        # reconfiguration."},{"line_number":932,"context_line":"        try:"},{"line_number":933,"context_line":"            self.executor.cancel(build)"},{"line_number":934,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"496bddcd_613521df","line":931,"updated":"2021-04-22 14:37:08.000000000","message":"I think we need to handle this without relying on a periodic cleanup.  We need to release nodes and semaphores immediately when we cancel jobs during a reconfiguration.\n\nAt a high level, we know every time we\u0027re going to cancel a job because we\u0027re removing the pipeline or tenant, so when we do that, we should delete the zk node record which will instruct the executor to abort the job.  Once we have builds in ZK, we should also have the executor holding the lock on the nodes, so once it is finished aborting, it can release the node lock which will return the nodes to nodepool.  But maybe that\u0027s not in the first step of implementation, so until then, we\u0027ll want to release the nodes in the scheduler too.  I\u0027m not sure it\u0027s practical for the executors to manage semaphores, so we\u0027ll probably want to continue to release them in the scheduler as well.\n\nIn short: for this change we need to actively cancel jobs, return nodes, and release semaphores when we cancel jobs for tenants or pipelines that are deleted.  And we know that none of the events from the executor for those builds will be processed, because they weren\u0027t processed before we canceled the job, and we know there won\u0027t be a result processor for that pipeline/tenant after we finish canceling the job.  So we know that we can take responsibility for them.\n\nLater, we might be able to drop the special handling of returning nodepool nodes.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c4075cf69e2e5ab4d191055bba9d35466256aeae","unresolved":false,"context_lines":[{"line_number":928,"context_line":"        # TODO (felix): The cancel call can be removed once we switch the"},{"line_number":929,"context_line":"        # builds to ZooKeeper. It\u0027s main purpose is to cancel/complete the"},{"line_number":930,"context_line":"        # runnig gearman job which otherwise breaks the tests in case of a"},{"line_number":931,"context_line":"        # reconfiguration."},{"line_number":932,"context_line":"        try:"},{"line_number":933,"context_line":"            self.executor.cancel(build)"},{"line_number":934,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"6367a12d_7f699b5d","line":931,"updated":"2021-04-12 21:39:32.000000000","message":"Just a note: We should still make sure the build is canceled in the future, even if the mechanism is different.  However it happens, we don\u0027t want it running after this point since it\u0027s effectively orphaned.\n\nAn actual concern: I think semaphores may not be handled in the case a pipeline or tenant is removed.  This method shares some code with scheduler.cancelJob, and that does return semaphores.  Perhaps we could use that instead of this?  Or otherwise somehow merge this code path?","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"10070016a9cdb024ce892134f1483e4d5eda468f","unresolved":false,"context_lines":[{"line_number":928,"context_line":"        # TODO (felix): The cancel call can be removed once we switch the"},{"line_number":929,"context_line":"        # builds to ZooKeeper. It\u0027s main purpose is to cancel/complete the"},{"line_number":930,"context_line":"        # runnig gearman job which otherwise breaks the tests in case of a"},{"line_number":931,"context_line":"        # reconfiguration."},{"line_number":932,"context_line":"        try:"},{"line_number":933,"context_line":"            self.executor.cancel(build)"},{"line_number":934,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"d22ef751_d20499d2","line":931,"updated":"2021-04-22 00:35:35.000000000","message":"Okay, deleting the build from zk directly sounds like a potentially good solution.\n\nWe still have some issues we need to address:\n\n1) The comment above is a little misleading -- it\u0027s not really to fix the tests, it fixes a real problem that the tests are showing us.\n\n2) We still need to deal with semaphores.\n\n3) This shares a lot in common with https://review.opendev.org/785536 which effectively changes reconfigurations so that the only things they can mutate are deleting pipelines or tenants.  I think we should rebase on top of that change (actually https://review.opendev.org/786744 which is the end of that stack) and work from there.\n\nOnce we rebase, we\u0027ll still have to address the conflict you point out: that sometimes we call cancelJob when a tenant+pipeline still exists, and sometimes we call it when we\u0027re deleting it.\n\nIf it\u0027s complicated to resolve that issue, and in the very next change we\u0027re going to move the build requests into ZK as well which makes the issue moot, then we may want to review those changes together (we don\u0027t need to squash them, we can leave them as two changes) and just make sure we merge them together.\n\nIf we can resolve it with a temporary method like this one, that\u0027s fine too.\n\nTo summarise:\n\n* Rebase on 786744\n* Analyze the pipeline/tenant cleanup situation in that case\n  * If it\u0027s not too difficult to resolve, keep this as a standalone change (we need to handle semaphores too).\n  * Otherwise, do something hacky to make the tests pass but review it with the next change and merge them together.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"738c66029bde7a77e53ee57c9980be9400af2372","unresolved":true,"context_lines":[{"line_number":928,"context_line":"        # TODO (felix): The cancel call can be removed once we switch the"},{"line_number":929,"context_line":"        # builds to ZooKeeper. It\u0027s main purpose is to cancel/complete the"},{"line_number":930,"context_line":"        # runnig gearman job which otherwise breaks the tests in case of a"},{"line_number":931,"context_line":"        # reconfiguration."},{"line_number":932,"context_line":"        try:"},{"line_number":933,"context_line":"            self.executor.cancel(build)"},{"line_number":934,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aef8585_757dee85","line":931,"in_reply_to":"6367a12d_7f699b5d","updated":"2021-04-15 14:24:08.000000000","message":"I tried that out, but it looks like cancelJob() is only returning the nodeset in case the build wasn\u0027t running. I think in case the build was running the \"normal\" mechanism is used when the build result event is processed. However, with the result events in ZK, the scheduler won\u0027t process any results for deleted pipelines/tenants anymore. That might also be the reason why I didn\u0027t use cancelJob() here in the first place and only called the relevant parts (deleting the entry in self.executor.builds and returnung the nodeset) directly. So maybe we should simply call the tenant.semaphore_handler.release(item, job) in here to also handle semaphores.\n\nRegarding the \"orphaned builds\": In the builds API change I simply delete the build in ZooKeeper directly (rather than invoking a cancel on the build) because the cancel event can\u0027t be processed anymore (see the comments in this method). The executor server listens for deleted build nodes and aborts the build in that case. The result would be the same as the executor server will then put a cancel event in the queue which isn\u0027t processed anymore, but IMHO it\u0027s a little more specific to simply delete the build (and return the nodeset) in here as this is all we need.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"4262222245f028289c9838cda851d01b9cac03b1","unresolved":true,"context_lines":[{"line_number":928,"context_line":"        # TODO (felix): The cancel call can be removed once we switch the"},{"line_number":929,"context_line":"        # builds to ZooKeeper. It\u0027s main purpose is to cancel/complete the"},{"line_number":930,"context_line":"        # runnig gearman job which otherwise breaks the tests in case of a"},{"line_number":931,"context_line":"        # reconfiguration."},{"line_number":932,"context_line":"        try:"},{"line_number":933,"context_line":"            self.executor.cancel(build)"},{"line_number":934,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":3,"id":"0abeaf00_0e1bfb17","line":931,"in_reply_to":"d22ef751_d20499d2","updated":"2021-04-22 07:17:47.000000000","message":"1) I can adapt the comment. I just wanted to write down what I saw so that at least somebody understands what this is all about in the first place.\n\n2) I can re-use the line that returns the semaphores in the cancelJob() method. I\u0027m just not sure if that is enough or if there is something else to address since I\u0027m not familiar with semaphores at all.\n\n3) Regarding your concern here\n\u003e Once we rebase, we\u0027ll still have to address the conflict you point out: that sometimes we call cancelJob when a tenant+pipeline still exists, and sometimes we call it when we\u0027re deleting it.\n\nI think we need to implement some periodic cleanup for most of the ZooKeeper stuff. This could then also deal with that use case. Apart from this I think it\u0027s quite difficult to solve this issue as there could always be a race between the reconfiguration and a running job (unless we ensure that all jobs are finished before the reconfig starts, but I think this is rather difficult and not what we want). We might also be able to process the result events from all tenants/pipelines (not just the active ones), but that doesn\u0027t sound right. IMHO it wouldn\u0027t make much sense to process those results since nobody would be able to use them.","commit_id":"0a026afe3cca21dc7f5a7500d3110d90f4990b23"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2abd19f3cac6caf46efaff31745eeda8b957d2e3","unresolved":false,"context_lines":[{"line_number":1009,"context_line":"        # Delete the tenant root path for this tenant in ZooKeeper to remove"},{"line_number":1010,"context_line":"        # all tenant specific event queues"},{"line_number":1011,"context_line":"        try:"},{"line_number":1012,"context_line":"            self.zk_client.client.delete(f\"{TENANT_EVENT_ROOT}/{tenant.name}\","},{"line_number":1013,"context_line":"                                         recursive\u003dTrue)"},{"line_number":1014,"context_line":"        except NotEmptyError:"},{"line_number":1015,"context_line":"            # In case a build result has been submitted during the"}],"source_content_type":"text/x-python","patch_set":7,"id":"ea0c9e5b_299f1678","line":1012,"updated":"2021-04-27 13:46:54.000000000","message":"I\u0027d rather not rename things like that on import without a good reason; it makes grepping difficult or impossible.  We should probably rename it in the original to make it more clear.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"d6ca9a84a567a5116128cf95b512e62abcf10f7c","unresolved":true,"context_lines":[{"line_number":1009,"context_line":"        # Delete the tenant root path for this tenant in ZooKeeper to remove"},{"line_number":1010,"context_line":"        # all tenant specific event queues"},{"line_number":1011,"context_line":"        try:"},{"line_number":1012,"context_line":"            self.zk_client.client.delete(f\"{TENANT_EVENT_ROOT}/{tenant.name}\","},{"line_number":1013,"context_line":"                                         recursive\u003dTrue)"},{"line_number":1014,"context_line":"        except NotEmptyError:"},{"line_number":1015,"context_line":"            # In case a build result has been submitted during the"}],"source_content_type":"text/x-python","patch_set":7,"id":"e91c025f_11a7774a","line":1012,"range":{"start_line":1012,"start_character":51,"end_line":1012,"end_character":56},"updated":"2021-04-26 22:55:17.000000000","message":"The import above is for TENANT_ROOT not TENANT_EVENT_ROOT.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"80da4bd3dc66535f2105e6f1615a8c18e0a21531","unresolved":true,"context_lines":[{"line_number":1009,"context_line":"        # Delete the tenant root path for this tenant in ZooKeeper to remove"},{"line_number":1010,"context_line":"        # all tenant specific event queues"},{"line_number":1011,"context_line":"        try:"},{"line_number":1012,"context_line":"            self.zk_client.client.delete(f\"{TENANT_EVENT_ROOT}/{tenant.name}\","},{"line_number":1013,"context_line":"                                         recursive\u003dTrue)"},{"line_number":1014,"context_line":"        except NotEmptyError:"},{"line_number":1015,"context_line":"            # In case a build result has been submitted during the"}],"source_content_type":"text/x-python","patch_set":7,"id":"c3f2eb64_d0e0b373","line":1012,"range":{"start_line":1012,"start_character":51,"end_line":1012,"end_character":56},"in_reply_to":"e91c025f_11a7774a","updated":"2021-04-27 05:13:49.000000000","message":"I did the following import in my version of the change:\n\nfrom zuul.zk.event_queues import TENANT_ROOT as TENANT_EVENT_ROOT\n\nThe main reason was to make the delete part that uses this variable further down a little more understandable. We are not removing the whole tenant there, but just the event queues in ZooKeeper. Maybe we will change the variable name in the event_queues module later on as well, but I didn\u0027t want to do this in here.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"d6ca9a84a567a5116128cf95b512e62abcf10f7c","unresolved":true,"context_lines":[{"line_number":1430,"context_line":"        for tenant in self.abide.tenants.values():"},{"line_number":1431,"context_line":"            for pipeline in tenant.layout.pipelines.values():"},{"line_number":1432,"context_line":"                for event in self.pipeline_result_events[tenant.name]["},{"line_number":1433,"context_line":"                    pipeline.name"},{"line_number":1434,"context_line":"                ]:"},{"line_number":1435,"context_line":"                    if self._stopped:"},{"line_number":1436,"context_line":"                        return"}],"source_content_type":"text/x-python","patch_set":7,"id":"68870a82_948cc368","line":1433,"updated":"2021-04-26 22:55:17.000000000","message":"Any concern that a busy tenant could starve the others? Maybe we should consider a breadth first approach? I though I suspect depth first like this is better most of the time.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"4bc8c52ea10c036fa184fff70b79b4baff3766b8","unresolved":false,"context_lines":[{"line_number":1430,"context_line":"        for tenant in self.abide.tenants.values():"},{"line_number":1431,"context_line":"            for pipeline in tenant.layout.pipelines.values():"},{"line_number":1432,"context_line":"                for event in self.pipeline_result_events[tenant.name]["},{"line_number":1433,"context_line":"                    pipeline.name"},{"line_number":1434,"context_line":"                ]:"},{"line_number":1435,"context_line":"                    if self._stopped:"},{"line_number":1436,"context_line":"                        return"}],"source_content_type":"text/x-python","patch_set":7,"id":"cf97d4a4_d69c25ca","line":1433,"updated":"2021-04-26 23:09:24.000000000","message":"When there are multiple schedulers, we\u0027ll lock for processing at the pipeline level.  So this is probably the way to go to be most efficient then.  And of course, if there is starvation in that case, another scheduler can be added.\n\nAlso, I think since we get the event list at the start of the iterator, we\u0027ll eventually move on to other pipelines and tenants, so a busy tenant could cause other tenants to \u0027pause\u0027 but not completely starve them.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"4bc8c52ea10c036fa184fff70b79b4baff3766b8","unresolved":false,"context_lines":[{"line_number":1691,"context_line":"                build.retry \u003d True"},{"line_number":1692,"context_line":"        if result in (\"DISCONNECT\", \"ABORTED\"):"},{"line_number":1693,"context_line":"            # Always retry if the executor just went away"},{"line_number":1694,"context_line":"            build.retry \u003d True"},{"line_number":1695,"context_line":"        if result \u003d\u003d \"MERGER_FAILURE\":"},{"line_number":1696,"context_line":"            # The build result MERGER_FAILURE is a bit misleading here"},{"line_number":1697,"context_line":"            # because when we got here we know that there are no merge"}],"source_content_type":"text/x-python","patch_set":7,"id":"adebf377_d7551bab","line":1694,"updated":"2021-04-26 23:09:24.000000000","message":"I think this is a straight copy from the existing executor client and not a change in logic.\n\nRegardless, I think this is correct -- an executor crashing shouldn\u0027t penalize the job.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"d6ca9a84a567a5116128cf95b512e62abcf10f7c","unresolved":true,"context_lines":[{"line_number":1691,"context_line":"                build.retry \u003d True"},{"line_number":1692,"context_line":"        if result in (\"DISCONNECT\", \"ABORTED\"):"},{"line_number":1693,"context_line":"            # Always retry if the executor just went away"},{"line_number":1694,"context_line":"            build.retry \u003d True"},{"line_number":1695,"context_line":"        if result \u003d\u003d \"MERGER_FAILURE\":"},{"line_number":1696,"context_line":"            # The build result MERGER_FAILURE is a bit misleading here"},{"line_number":1697,"context_line":"            # because when we got here we know that there are no merge"}],"source_content_type":"text/x-python","patch_set":7,"id":"43e16754_17630d16","line":1694,"updated":"2021-04-26 22:55:17.000000000","message":"Shouldn\u0027t this be subject to the build.job.attempts upper limit of retries too?","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"80da4bd3dc66535f2105e6f1615a8c18e0a21531","unresolved":false,"context_lines":[{"line_number":1691,"context_line":"                build.retry \u003d True"},{"line_number":1692,"context_line":"        if result in (\"DISCONNECT\", \"ABORTED\"):"},{"line_number":1693,"context_line":"            # Always retry if the executor just went away"},{"line_number":1694,"context_line":"            build.retry \u003d True"},{"line_number":1695,"context_line":"        if result \u003d\u003d \"MERGER_FAILURE\":"},{"line_number":1696,"context_line":"            # The build result MERGER_FAILURE is a bit misleading here"},{"line_number":1697,"context_line":"            # because when we got here we know that there are no merge"}],"source_content_type":"text/x-python","patch_set":7,"id":"1709825f_86deda34","line":1694,"in_reply_to":"adebf377_d7551bab","updated":"2021-04-27 05:13:49.000000000","message":"Yes the whole result event handling is just copy/move of the existing parts from different places. I did just a few adaptions to make the parts work together in a single place.","commit_id":"b44222e3697070e296a8630bb53ded80433a3067"}]}