-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Worker removes flow_run id if fails to submit. #16561
base: main
Are you sure you want to change the base?
Worker removes flow_run id if fails to submit. #16561
Conversation
CodSpeed Performance ReportMerging #16561 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to add a regression test in addition to the suggested changes to the log levels.
src/prefect/workers/base.py
Outdated
@@ -972,6 +975,7 @@ async def _submit_run(self, flow_run: "FlowRun") -> None: | |||
|
|||
self._submitting_flow_run_ids.remove(flow_run.id) | |||
else: | |||
self._submitting_flow_run_ids.remove(flow_run.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at this more closely, since we're removing from _submitting_flow_run_ids
in both branches, you simplify this by moving the removal outside of the if/else
statement like this:
if ready_to_submit:
...
else:
self._release_limit_slot(flow_run.id)
self._submitting_flow_run_ids.remove(flow_run.id)
…stuck-in' of https://github.com/PrefectHQ/prefect into jean/oss-5929-deployment-concurrency-limit-leaves-jobs-stuck-in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR fixes but where flow runs would be stuck in the
AwaitingConcurrency
if a concurreny limit was set at the deployment level. The bug can be summarized into:_submitting_flow_run_ids
AwaitingConcurrency
therefore makingready_to_submit=False
_submit_scheduled_flow_runs
never resubmits flow because it still finds it in_submitting_flow_run_ids
closes:Deployment concurrency limit leaves jobs stuck in AwaitingConcurrencySlot #16027
Checklist
<link to issue>
"mint.json
.