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

Boefjes combined schedulers integration #4015

Open
wants to merge 3 commits into
base: poc/mula/combined-schedulers
Choose a base branch
from

Conversation

jpbruinsslot
Copy link
Contributor

@jpbruinsslot jpbruinsslot commented Jan 8, 2025

Changes

Warning

Please check the SOURCE PR and the associated issue(s) to get the background and full context of this PR. This PR is to be merged into that source PR.

  • Since the /queues has been refactored to the /schedulers/{scheduler_id}/pop endpoint how the task runner is popping off tasks. We don't have to iterate over all the organizational queues and check if items are still on the queue. We now can just pop off tasks from the queue.
  • Updates to the model to resemble the changes that have been made in the scheduler

Issue link

Closes #3961

@jpbruinsslot jpbruinsslot self-assigned this Jan 8, 2025
@jpbruinsslot jpbruinsslot requested a review from a team as a code owner January 8, 2025 11:18
@jpbruinsslot jpbruinsslot changed the base branch from poc/mula/combined-schedulers to main January 8, 2025 11:24
@jpbruinsslot jpbruinsslot changed the base branch from main to poc/mula/combined-schedulers January 8, 2025 11:24
@jpbruinsslot jpbruinsslot added the boefjes Issues related to boefjes label Jan 8, 2025
@jpbruinsslot jpbruinsslot marked this pull request as draft January 8, 2025 14:10
# TODO: check
if not response:
logger.debug("Queue %s empty", queue_type.value)
time.sleep(10)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be time.sleep(self.settings.poll_interval) ?

if not p_item:
logger.debug("Queue %s empty", queue.id)
continue
# TODO: check
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check what kind of response the scheduler gives when the queue is empty and handle this.

Comment on lines +84 to +91
def pop_item(self, scheduler_id: str) -> PaginatedTasksResponse | None:
response = self._session.post(f"/schedulers/{scheduler_id}/pop?limit=1")
self._verify_response(response)

return TypeAdapter(PaginatedTasksResponse | None).validate_json(response.content)

def pop_items(self, scheduler_id: str, filters: dict[str, Any]) -> PaginatedTasksResponse | None:
response = self._session.post(f"/schedulers/{scheduler_id}/pop", json=filters)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important to note is that the pop endpoint changed to return multiple Tasks, additionally it allows for more complex filtering options. I've, for now limited the response to 1 Task to resemble how it currently works, but this can be updated accordingly to what is desired.

@jpbruinsslot jpbruinsslot requested a review from Donnype January 8, 2025 14:27
@jpbruinsslot jpbruinsslot marked this pull request as ready for review January 8, 2025 14:31
if task_queue.qsize() > self.settings.pool_size:
time.sleep(self.settings.worker_heartbeat)
return
logger.debug("Popping from queue %s", queue_type.value)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this back in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boefjes Issues related to boefjes
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

1 participant