-
Notifications
You must be signed in to change notification settings - Fork 7
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
DATAUP-544 API Defined #420
base: develop
Are you sure you want to change the base?
Changes from 10 commits
bdf5180
7011bfe
3133d58
ac8da69
9adfa50
2d3f5ee
e15ceb1
6e35b8d
586f6a4
b4bab18
6f9e6f9
79432c8
4f0c4ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,8 @@ | |
import traceback | ||
from contextlib import contextmanager | ||
from datetime import datetime | ||
from typing import Dict, List | ||
from typing import Dict, List, Union | ||
|
||
from bson.objectid import ObjectId | ||
from mongoengine import connect, connection | ||
from pymongo import MongoClient, UpdateOne | ||
|
@@ -15,9 +16,8 @@ | |
RecordNotFoundException, | ||
InvalidStatusTransitionException, | ||
) | ||
|
||
from lib.execution_engine2.utils.arg_processing import parse_bool | ||
from execution_engine2.sdk.EE2Runjob import JobIdPair | ||
from lib.execution_engine2.utils.arg_processing import parse_bool | ||
|
||
|
||
class MongoUtil: | ||
|
@@ -223,6 +223,48 @@ def get_job(self, job_id=None, exclude_fields=None) -> Job: | |
|
||
return job | ||
|
||
def get_jobs_with_status( | ||
self, | ||
job_ids: List[str], | ||
status_list: List[str], | ||
only_job_ids: bool = False, | ||
retried_jobs_allowed=True, | ||
) -> Union[List[Job], List[str]]: | ||
if not (job_ids and isinstance(job_ids, list)): | ||
raise ValueError("Please provide a non empty list of job ids") | ||
|
||
if not (status_list and isinstance(job_ids, list)): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should that second one be It would be good to put in some tests for this so you can check what is and isn't caught by these conditionals vs by the type checking in the function definition. It's sometimes easier to have arguments that should be a list default to |
||
raise ValueError("Please provide a non empty list of job statuses") | ||
|
||
with self.mongo_engine_connection(): | ||
# TODO: Only seems to be returning other fields as well. Is .only() broken? | ||
if retried_jobs_allowed: | ||
jobs = Job.objects(id__in=job_ids, status__in=status_list) | ||
else: | ||
jobs = Job.objects( | ||
id__in=job_ids, status__in=status_list, retry_parent__exists=False | ||
) | ||
Comment on lines
+244
to
+246
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is nice! is it going to be possible to query ee2 for jobs by cell_id at some point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't tried it but there's a check_job_date_range_by_user function that might let you filter on that field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought it might make more sense to change check_jobs_wsid() to have a filter for specific fields |
||
|
||
if only_job_ids: | ||
return [str(job.id) for job in jobs] | ||
return jobs | ||
|
||
def eligible_for_retry(self, job: Job): | ||
""" | ||
Checks the job record to see if it has any retry_ids, | ||
and if those retry_ids do not contain an ineligble job state | ||
:param job: | ||
:return: | ||
""" | ||
if job.retry_ids == []: | ||
return True | ||
eligble_states = [Status.completed.value, Status.error.value] | ||
ialarmedalien marked this conversation as resolved.
Show resolved
Hide resolved
|
||
jobs = self.get_jobs(job_ids=job.retry_ids) | ||
for job in jobs: | ||
if job.status not in eligble_states: | ||
return False | ||
return True | ||
|
||
def get_jobs( | ||
self, job_ids=None, exclude_fields=None, sort_id_ascending=None | ||
) -> List[Job]: | ||
|
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.
if not supplying a status_list is going to throw an exception, the function name should reflect that -- e.g.
retry_batch_jobs_by_status
andcancel_batch_jobs_by_status
. If you want to keep the API names as-is, I would recommend havingretry_batch_jobs
default to retrying all jobs of the appropriate status (i.e. terminated or error).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.
@ialarmedalien What is your preferred behavior?
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.
Name the function
(retry|cancel)_batch_jobs_by_status
and if you don't supply a list of statuses, it throws an exception.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.
Renamed, but shouldn't it be cancel_batch_job_by_status?