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

Update server logic to select jobs with high priority #375

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

val500
Copy link
Contributor

@val500 val500 commented Oct 10, 2024

Description

This changes the query to get a job from the database to sort by job_priority first so jobs with highest priority get selected first. In addition, find_one_and_update was replaced by a find_one query and a separate update query. This is due to MongoDB not supporting the sort option for updates leading to a mismatch between the document that was found and the document that was updated.

Resolved issues

Resolves https://warthogs.atlassian.net/browse/CERTTF-373

Documentation

Web service API changes

N/A

Tests

Unit test was added to test_v1.py

@val500 val500 requested a review from plars October 10, 2024 19:13
Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

A few small things, but looking good.

@@ -107,6 +107,7 @@ def create_indexes():
# Faster lookups for common queries
mongo.db.jobs.create_index("job_id")
mongo.db.jobs.create_index(["result_data.job_state", "job_data.job_queue"])
mongo.db.jobs.create_index(["job_priority", -1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we'll actually need the index here. We're already indexing result_data.job_state and job_data.job_queue and filtering the db for the ones that match the queue we want and where state=="waiting", so this should already be a very small list.

@@ -176,7 +177,7 @@ def pop_job(queue_list):
"""Get the next job in the queue"""
# The queue name and the job are returned, but we don't need the queue now
try:
response = mongo.db.jobs.find_one_and_update(
response = mongo.db.jobs.find_one(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure there's no way to still do this as a single operation? Doing the atomic find_one_and_update allows us to ensure that there's no race where another agent picks up the same job, so it would be nice if we could keep this if possible.

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 was getting a weird issue in my test-case where the item that was returned in response was different from the item that was updated which was because I saw that update_one did not support that sort key, but on the MongoDB documentation, I see that the sort key functionality was added in v8.0(https://www.mongodb.com/docs/manual/reference/method/db.collection.findOneAndUpdate). Are we using v8.0 or does the mongomock also support this?

Copy link
Contributor Author

@val500 val500 Oct 11, 2024

Choose a reason for hiding this comment

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

I think this might be an issue with MongoMock:
https://github.com/mongomock/mongomock/blob/d821f320dbf91432a09a42dc200d7306ec5a6244/mongomock/collection.py#L1348

If you look at the implementation of find_one_and_update in MongoMock, there's two separate calls to find_one and update - the sort parameter is passed to the find_one function but not the update function.

Edit:
The query seems to get reupdated once mongomock calls find_one, but it only does this if "_id" is present in the response, which we explicitly don't include in the projection.

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