Fix ticket-based job state transition for analysis jobs #1105
Conversation
|
@nagem @ambrussimon Please confirm if this adds any risk for endpoints in use, so we can decide whether to hold this merge for after the imminent release. Thanks. |
ambrussimon
force-pushed
the
analysis-job-completion-ticket
branch
from
1f566ab to
dd7bc31
Compare
Codecov Report
@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
+ Coverage 90.92% 90.98% +0.05%
==========================================
Files 49 49
Lines 7055 7068 +13
==========================================
+ Hits 6415 6431 +16
+ Misses 640 637 -3 |
|
@ryansanford Change scope is limited to |
| }) | ||
| Queue.mutate(job, {'state': 'complete' if success else 'failed', | ||
| 'profile': {'elapsed': job_ticket['elapsed']}}) | ||
| job = Job.get(job.id_) |
There was a problem hiding this comment.
This will raise an APINotFoundException, not return None.
It looks like we call it above too so I'm curious why you're checking for None below. Race condition that it gets deleted between the Queue.mutate call and save?
There was a problem hiding this comment.
Queue.mutate does not update the loaded job's attributes (except partially in this special case: https://github.com/scitran/core/blob/master/api/jobs/queue.py#L82)
Therefore calling job.save() once more (when setting saved_files below) would try to revert the state transition.
Was thinking of updating mutate instead, but wanted to limit the scope of the change and the possible errors introduced.
There was a problem hiding this comment.
+1 on that, I ran into that edge case myself earlier.
ambrussimon
force-pushed
the
analysis-job-completion-ticket
branch
from
dd7bc31 to
03784df
Compare
| file_attrs['created'] = file_attrs['modified'] | ||
| self.save_file(field) | ||
| self.saved.append(file_attrs) | ||
|
|
||
| def finalize(self): | ||
| # Search the sessions table for analysis, replace file field | ||
| if self.saved: |
There was a problem hiding this comment.
Sanity check: you're sure we don't need the self.saved conditional? I'm not super familiar here.
Fixes #1084
Review Checklist