DATAUP-544 API Defined #420
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #420 +/- ##
===========================================
+ Coverage 83.84% 88.19% +4.35%
===========================================
Files 22 29 +7
Lines 1993 2864 +871
===========================================
+ Hits 1671 2526 +855
- Misses 322 338 +16
Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging 2d3f5ee into 96b6f69 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging e15ceb1 into ee0d641 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b4bab18 into ee0d641 - view on LGTM.com new alerts:
|
|
execution_engine2.spec
Outdated
| If no status_list is provided, an exception is thrown. | ||
| */ | ||
| funcdef retry_batch_jobs(BatchRetryParams params) returns (list<RetryResult> retry_result) authentication required; |
There was a problem hiding this comment.
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 and cancel_batch_jobs_by_status. If you want to keep the API names as-is, I would recommend having retry_batch_jobs default to retrying all jobs of the appropriate status (i.e. terminated or error).
There was a problem hiding this comment.
@ialarmedalien What is your preferred behavior?
There was a problem hiding this comment.
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.
Renamed, but shouldn't it be cancel_batch_job_by_status?
|
This pull request introduces 1 alert when merging 6f9e6f9 into 1e268df - view on LGTM.com new alerts:
|
| 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.
should that second one be isinstance(status_list, list)?
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 None so that you don't have to check the length of the list.
| jobs = Job.objects( | ||
| id__in=job_ids, status__in=status_list, retry_parent__exists=False | ||
| ) |
There was a problem hiding this comment.
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.
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.
Thought it might make more sense to change check_jobs_wsid() to have a filter for specific fields
| """ | ||
| 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: Should be a child job of a BATCH job |
There was a problem hiding this comment.
should that check be enforced in the code here? Should be relatively simple just to add in a check for whether the batch_id field is present and the batch_job field is false.
| @@ -758,6 +760,62 @@ def retry(self, job_id: str, as_admin=False) -> Dict[str, Optional[str]]: | |||
| job_id=job_id, job=job, batch_job=batch_job, as_admin=as_admin | |||
| ) | |||
|
|
|||
| def retry_jobs_in_batch_by_status( | |||
| self, job_id: str, status_list: list, as_admin: bool = False | |||
There was a problem hiding this comment.
type specification (List[str]) and default for status_list?
| # Retry and Report | ||
| # Get jobs that | ||
| # do NOT have a retry_parent, i.e. only jobs that haven't been retried | ||
| # and jobs that have not retried retry_parents only |
There was a problem hiding this comment.
this comment is a little confusing...
|
|
||
| # Termination | ||
| terminated_ids = [] | ||
| child_job_ids = self.sdkmr.get_mongo_util().get_jobs_with_status( |
There was a problem hiding this comment.
minor point -- if you're returning job objects, call the variable child_jobs -- child_job_ids sounds like you're returning IDs only, but the next few lines show that you've got job objects.
| funcdef cancel_batch_jobs_by_status(BatchCancelParams params) returns (list<job_id> job_ids) authentication required; | ||
|
|
There was a problem hiding this comment.
if no jobs are in the appropriate states, the endpoint returns [] -- is that correct?
| # TEST OUT ERROR CONDITIONS | ||
| for status_list in [ | ||
| None, | ||
| [], | ||
| ]: |
There was a problem hiding this comment.
I think it'd be easier to understand if you split out all these tests into separate functions -- e.g. test_retry_batch__no_batch_id, test_retry_batch__no_status_list, test_retry_batch__invalid_status_list, test_retry_batch__invalid_batch_id, test_retry_batch__no_jobs_with_status, etc., etc. Stepping through the functions and ensuring there are clearly-named tests in all the appropriate places will make it a lot easier to track down issues.
Description of PR purpose/changes
Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)