Skip to content
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

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

infherny
Copy link
Contributor

The goal of this MR is to add the ability to lock jobs by executors.

src/saturn_engine/default_config.py Outdated Show resolved Hide resolved
src/saturn_engine/worker/topics/rabbitmq.py Outdated Show resolved Hide resolved
@@ -57,9 +57,15 @@ def lock_jobs(
)
)

# Join definitions and filtered out by executors
Copy link
Member

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.

Copy link
Contributor Author

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,
            )
        )

Copy link
Contributor Author

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

@infherny infherny force-pushed the olivier/select_jobs_by_executors branch 2 times, most recently from 3ec8a4f to 19da130 Compare August 20, 2024 14:28
@infherny infherny force-pushed the olivier/select_jobs_by_executors branch from 19da130 to be83064 Compare August 20, 2024 14:47
@infherny infherny requested a review from isra17 August 20, 2024 15:06
@infherny infherny self-assigned this Aug 20, 2024
@infherny infherny merged commit 1640633 into main Aug 20, 2024
3 checks passed
@infherny infherny deleted the olivier/select_jobs_by_executors branch August 20, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants