Optimize the /list fetching code to not round-trip to UUID. #848

scrosby · 2018-05-14T21:07:43Z

Changes proposed in this PR

Don't round-trip jobs to UUID's then back to job entities in the /list path. Get the entities and populate them.

Why are we making these changes?

We think it'll be a modest performance increase; should avoid an extra random IO per job in the result set.

dposada · 2018-05-29T18:35:27Z

scheduler/src/cook/mesos/api.clj

+      (histograms/update! list-response-job-count (count job-ents))
+      job-ents)))
+
+(defn list-jobs


After this change, does anything call list-jobs?

Yes; Line 1300, jobs-list-exists?

Shouldn't that path also switch to using list-jobents?

Its a lot messier/riskier because the result of list-jobs is just stuffed into ::jobs in the context, and being a global, we have to trace the context through all of the code paths; pretty much, refactor the entire /jobs endpoint dataflow (individual UUID's, /job search parameters, etc). Now, the dataflow is entirely wired to generating, and consuming uuid's. If we push entities further down, we'd have to rewire the whole /jobs set of endpoints thing to entities, or clone chunks of it.

Sure, but /list is the deprecated endpoint, and /jobs is the "new" endpoint. It seems like if we're choosing to improve one but not the other, we should choose /jobs

DaoWen · 2018-05-29T21:35:38Z

This PR partially addresses issue #617.

scrosby · 2018-05-31T15:42:03Z

This PR addresses #790. The additional work on the /jobs involves the liberator context and addresses part of #617.

scrosby · 2018-05-31T16:03:42Z

@dposada Is there a unit test /jobs/#UUID that exeercises this codepath for https://github.com/twosigma/Cook/pull/848/files#diff-6611b39b65ff49438a4d940b51c85cc0R2549 ?
I didn't find one in a search, nor on your commit d236ec3 #756
I'd like to validate that this works correctly.

dposada · 2018-05-31T16:25:22Z

@scrosby The util.load_job function:

Cook/integration/tests/cook/util.py

Lines 521 to 523 in 204ddc8

    
           def load_job(cook_url, job_uuid, assert_response=True): 
        
               """Loads a job by UUID using GET /jobs/UUID""" 
        
               return load_resource(cook_url, 'jobs', job_uuid, assert_response)

which is used by several integration tests, exercises /jobs/UUID; let me know if that's what you're looking for.

…ext for /jobs endpoint. Avoids the same from-uuid to-uuid round-trip in the /jobs endpoint. Stored in ::jobs-entities.

dposada · 2018-06-05T18:27:39Z

scheduler/src/cook/mesos/api.clj

@@ -891,6 +890,10 @@
              progress-regex-string (assoc :progress-regex-string progress-regex-string)
              pool (assoc :pool (:pool/name pool))))))

+(defn fetch-job-map
+  [db framework-id job-uuid]
+  (fetch-job-map-from-entity db framework-id (d/entity db [:job/uuid job-uuid])))


Does anything still use fetch-job-map after this change?

Over a dozen uses.

What are they? Should they switch over to using the new function?

render-jobs-for-response-deprecated -- /rawscheduler handler
render-instances-for-response
11 times as a convenience function in unit tests.

Should we have render-jobs-for-response-deprecated and render-instances-for-response use the new way?

Thats a scope question..

If we do that, then effectively we're effectively rescoping this PR to do all of #617. Do we want to do that?

I would vote to either do that in this PR or do it immediately after this in a follow-up PR. Basically, I wouldn't want the two functions to stay around for very long.

dposada · 2018-06-05T18:28:39Z

scheduler/src/cook/mesos/api.clj

@@ -1116,22 +1119,31 @@
  (mapv (partial fetch-job-map (db conn) framework-id) (::jobs ctx)))

 (defn render-jobs-for-response
+  "This rendes for response. Fills in in :group UUID's and names as well as map-ifies jobs
+  It will examine the ctx for ::jobs (containing UUID's) or ::job-entities (containing
+  datomic job entities) and return the merged set."


Why do we need both ::jobs and ::job-entities?

::jobs has UUID's (coming from the /job/ and from /job&uuid=XXXX&uuid=XXXX) and ::job-entities has entities. Both have different logic as they filter through read-jobs-handler, so are separated until the very end.

Is it possible to have them work the same way so that we don't need both entries in the context map?

Yes, if we refactor the /jobs API endpoint implementation. It'll take time for me to get up to speed on liberator's semantics, because I'm going to have to face the ::params ::query-params, etc. All of the filtering and other functions in the &uuid=XXXX are all wired to UUID's. And if we're going to do that, we could greatly simplify it the implementation & semantics with a small API change. IMO, the current code is fighting with liberator, which is a sign that the API we're implementing isn't an appropriate match for it. Thus the proposal for a change.

Do we want to rescope this PR that much?

Thus the proposal for a change.

It's not clear to me what the change is that you're proposing.

dposada · 2018-06-20T14:47:36Z

@scrosby Do we want to keep this PR open?

dposada · 2018-06-26T14:08:33Z

@scrosby Can we close this PR?

scrosby added the wip label May 14, 2018

scrosby added wip and removed wip labels May 21, 2018

Optimize the job fetching code to not round-trip to UUID.

ce1212f

scrosby force-pushed the outgoing/faster_list branch from a87ebee to ce1212f Compare May 29, 2018 15:53

scrosby removed the wip label May 29, 2018

scrosby requested a review from dposada May 29, 2018 15:58

dposada reviewed May 29, 2018

View reviewed changes

scrosby self-assigned this May 30, 2018

Scott Crosby added 2 commits June 4, 2018 14:10

Address part of twosigma#617, pass job entities in the liberator cont…

9a2d3fa

…ext for /jobs endpoint. Avoids the same from-uuid to-uuid round-trip in the /jobs endpoint. Stored in ::jobs-entities.

Rename list-jobents to list-jobs

4cbd228

scrosby force-pushed the outgoing/faster_list branch from a9b04b3 to 4cbd228 Compare June 4, 2018 19:14

scrosby changed the title ~~WIP: Optimize the /list fetching code to not round-trip to UUID.~~ Optimize the /list fetching code to not round-trip to UUID. Jun 5, 2018

dposada reviewed Jun 5, 2018

View reviewed changes

pschorf closed this Jun 27, 2018

scrosby deleted the outgoing/faster_list branch November 12, 2018 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize the /list fetching code to not round-trip to UUID. #848

Optimize the /list fetching code to not round-trip to UUID. #848

scrosby commented May 14, 2018 •

edited

Loading

dposada May 29, 2018

scrosby May 30, 2018

dposada May 30, 2018

scrosby May 30, 2018 •

edited

Loading

dposada May 30, 2018 •

edited

Loading

DaoWen commented May 29, 2018

scrosby commented May 31, 2018 •

edited

Loading

scrosby commented May 31, 2018

dposada commented May 31, 2018 •

edited

Loading

dposada Jun 5, 2018

scrosby Jun 5, 2018

dposada Jun 5, 2018

scrosby Jun 5, 2018

dposada Jun 5, 2018

scrosby Jun 5, 2018 •

edited

Loading

dposada Jun 6, 2018

dposada Jun 5, 2018

scrosby Jun 5, 2018

dposada Jun 5, 2018

scrosby Jun 5, 2018 •

edited

Loading

dposada Jun 6, 2018

dposada commented Jun 20, 2018

dposada commented Jun 26, 2018

Optimize the /list fetching code to not round-trip to UUID. #848

Optimize the /list fetching code to not round-trip to UUID. #848

Conversation

scrosby commented May 14, 2018 • edited Loading

Changes proposed in this PR

Why are we making these changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrosby May 30, 2018 • edited Loading

Choose a reason for hiding this comment

dposada May 30, 2018 • edited Loading

Choose a reason for hiding this comment

DaoWen commented May 29, 2018

scrosby commented May 31, 2018 • edited Loading

scrosby commented May 31, 2018

dposada commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrosby Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrosby Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dposada commented Jun 20, 2018

dposada commented Jun 26, 2018

scrosby commented May 14, 2018 •

edited

Loading

scrosby May 30, 2018 •

edited

Loading

dposada May 30, 2018 •

edited

Loading

scrosby commented May 31, 2018 •

edited

Loading

dposada commented May 31, 2018 •

edited

Loading

scrosby Jun 5, 2018 •

edited

Loading

scrosby Jun 5, 2018 •

edited

Loading