Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce1212f
Optimize the job fetching code to not round-trip to UUID.
Mar 28, 2018
9a2d3fa
Address part of #617, pass job entities in the liberator context for …
Jun 4, 2018
4cbd228
Rename list-jobents to list-jobs
Jun 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 23 additions & 16 deletions scheduler/src/cook/mesos/api.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,12 +837,11 @@
progress-message (assoc :progress_message progress-message)
sandbox-directory (assoc :sandbox_directory sandbox-directory))))

(defn fetch-job-map
[db framework-id job-uuid]
(defn fetch-job-map-from-entity
[db framework-id job]
(timers/time!
(timers/timer ["cook-mesos" "internal" "fetch-job-map"])
(let [job (d/entity db [:job/uuid job-uuid])
resources (util/job-ent->resources job)
(let [resources (util/job-ent->resources job)
groups (:group/_job job)
application (:job/application job)
expected-runtime (:job/expected-runtime job)
Expand Down Expand Up @@ -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])))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over a dozen uses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@scrosby scrosby Jun 5, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


(defn fetch-group-live-jobs
"Get all jobs from a group that are currently running or waiting (not complete)"
[db guuid]
Expand Down Expand Up @@ -1257,8 +1260,8 @@
(histograms/defhistogram [cook-mesos api list-request-param-limit])
(histograms/defhistogram [cook-mesos api list-response-job-count])

(defn list-jobs
"Queries using the params from ctx and returns the job uuids that were found"
(defn list-jobents
"Queries using the params from ctx and returns the jobs that were found as datomic entities."
[db include-custom-executor? ctx]
(timers/time!
list-endpoint
Expand All @@ -1273,20 +1276,24 @@
start-ms' (or start-ms (- end-ms (-> since-hours-ago t/hours t/in-millis)))
start (Date. ^long start-ms')
end (Date. ^long end-ms)
job-uuids (->> (timers/time!
job-ents (->> (timers/time!
fetch-jobs
(util/get-jobs-by-user-and-states db user states start end limit
name-filter-fn include-custom-executor? pool-name))
(sort-by :job/submit-time)
reverse
(map :job/uuid))
job-uuids (if (nil? limit)
job-uuids
(take limit job-uuids))]
reverse)
job-ents (if (nil? limit)
job-ents
(take limit job-ents))]
(histograms/update! list-request-param-time-range-ms (- end-ms start-ms'))
(histograms/update! list-request-param-limit limit)
(histograms/update! list-response-job-count (count job-uuids))
job-uuids)))
(histograms/update! list-response-job-count (count job-ents))
job-ents)))

(defn list-jobs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this change, does anything call list-jobs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; Line 1300, jobs-list-exists?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@scrosby scrosby May 30, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dposada dposada May 30, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Queries using the params from ctx and returns the job uuids that were found"
[db include-custom-executor? ctx]
(map :job/uuid (list-jobents db include-custom-executor? ctx)))

(defn jobs-list-exist?
[conn ctx]
Expand Down Expand Up @@ -2285,8 +2292,8 @@
:handle-ok (fn [ctx]
(timers/time!
(timers/timer ["cook-scheduler" "handler" "list-endpoint-duration"])
(let [job-uuids (list-jobs db false ctx)]
(mapv (partial fetch-job-map db framework-id) job-uuids))))))
(let [job-ents (list-jobents db false ctx)]
(mapv (partial fetch-job-map-from-entity db framework-id) job-ents))))))

;;
;; /unscheduled_jobs
Expand Down