-
Notifications
You must be signed in to change notification settings - Fork 5
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: lock jobs by executors #433
Conversation
@@ -57,9 +57,15 @@ def lock_jobs( | |||
) | |||
) | |||
|
|||
# Join definitions and filtered out by executors |
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.
This won't really work. You select all jobs from the database then filter them. But the get_unassigned_queues
returns up to N
jobs, so it might only select the wrong job and not return any job using your executor.
We probably need to do the other way around: read the potential jobs from the static_definitions
with our executor selector, then get_unassigned_queues
that match one of these job.
It's ugly and inneficient, but that's the quickest and most reliable hack for now.
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.
What about removing this limit when we specify an executor?
if len(assigned_items) < max_assigned_items:
assigned_items.extend(
queues_store.get_unassigned_queues(
session=session,
assigned_before=assignation_expiration_cutoff,
limit=max_assigned_items - len(assigned_items) if not lock_input.executiors,
selector=lock_input.selector,
)
)
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.
I implemented the limit suggestion
3ec8a4f
to
19da130
Compare
19da130
to
be83064
Compare
The goal of this MR is to add the ability to lock jobs by executors.