scitran / core

RESTful API
https://scitran.github.io
MIT License
18 stars 18 forks source link

Menagerie of job handler improvements #1062

Closed kofalt closed 6 years ago

kofalt commented 6 years ago
  1. Adding logs to a job will now unconditionally heartbeat that job first. A job consumer now has half the requests over time required when a job is running.
  2. Creating a job-completion ticket will prevent that job from being orphaned. A job consumer no longer has to keep heartbeating while an upload is in progress.
  3. For this reason, tickets now last an hour instead of 5 minutes.
  4. Job tickets are now their own class in jobs.py.
  5. A bunch of handlers were not using @directives or had ineffective return statements. Corrected.
  6. Moved some repetitive download logic to util.set_for_download.
  7. Moved some dumb humanizing logic to util.humanize_validation_error.
  8. Harmonized some, but not all, uses of self.response.json variable naming.
  9. Slightly reduced the log spam when jobs-next has nothing to return.
  10. Fixed a bug in the new job-ticket handler that was not marking jobs as complete / failed.
  11. Removed meaningless tempdir warnings. Congrats, CI is 288 lines shorter :tada:
codecov-io commented 6 years ago

Codecov Report

Merging #1062 into master will decrease coverage by 0.02%. The diff coverage is 95.19%.

@@            Coverage Diff             @@
##           master    #1062      +/-   ##
==========================================
- Coverage   90.82%   90.79%   -0.03%     
==========================================
  Files          50       50              
  Lines        7017     7019       +2     
==========================================
  Hits         6373     6373              
- Misses        644      646       +2
nagem commented 6 years ago

Creating a job-completion ticket will prevent that job from being orphaned. A job consumer no longer has to stop heartbeating while an upload is in progress.

Did you mean a job consumer will no longer have to keep heartbeating?

nagem commented 6 years ago

A bunch of handlers were not using @directives

👍 👍 👍

kofalt commented 6 years ago

Comments addressed, and small log tweak added.

9) Slightly reduced the log spam when jobs-next has nothing to return.

kofalt commented 6 years ago
  1. Fixed a bug in the new job-ticket handler that was not marking jobs as complete / failed.
  2. Removed meaningless tempdir warnings. Congrats, CI is 288 lines shorter :tada:
ryansanford commented 6 years ago

@kofalt @nagem Any final changes or review blocking the merge of this? Correct that there are still no breaking changes here?

I understand this PR to be a blocker to wrapping up the new Flywheel Engine.

nagem commented 6 years ago

@ryansanford Looks like more changes were added again so I’ll need to review those before it can merge.

kofalt commented 6 years ago

FWIW, this branch does not need to be a blocker; I have configured e2 to use the new job-complete ticketing that this branch modifies, but I made sure it is very easy to disable. Six character diff.

I can even make that configurable if desired... maybe I should, just in case. :thinking:

ryansanford commented 6 years ago

I'd prefer this PR to have final review and merge. Thanks for the flexibility though @kofalt

nagem commented 6 years ago

@ambrussimon do you mind taking a look at the last two commits if you have a chance? They reference your work with job completion tickets.