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 all commits
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
53 changes: 32 additions & 21 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 @@ -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."
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

::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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thus the proposal for a change.

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

[conn framework-id ctx]
(let [db (db conn)

fetch-group
(fn fetch-group [group-uuid]
(let [group (d/entity db [:group/uuid (UUID/fromString group-uuid)])]
{:uuid group-uuid
:name (:group/name group)}))

fetch-job
fetch-job-from-uuid
(fn fetch-job [job-uuid]
(let [job (fetch-job-map db framework-id job-uuid)
groups (mapv fetch-group (:groups job))]
(assoc job :groups groups)))
fetch-job-from-entity
(fn fetch-job [job-ent]
(let [job (fetch-job-map-from-entity db framework-id job-ent)
groups (mapv fetch-group (:groups job))]
(assoc job :groups groups)))]

(mapv fetch-job (::jobs ctx))))
(concat
(when-let [entities (::jobs-entities ctx)]
(mapv fetch-job-from-entity entities))
(when-let [uuids (::jobs ctx)]
(mapv fetch-job-from-uuid uuids)))))

(defn render-instances-for-response
[conn framework-id ctx]
Expand Down Expand Up @@ -1258,7 +1270,7 @@
(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"
"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,24 +1285,23 @@
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 jobs-list-exist?
[conn ctx]
[true {::jobs (list-jobs (d/db conn) true ctx)}])
[true {::jobs-entities (list-jobs (d/db conn) true ctx)}])

(defn read-jobs-handler
[conn is-authorized-fn resource-attrs]
Expand Down Expand Up @@ -2285,8 +2296,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-jobs db false ctx)]
(mapv (partial fetch-job-map-from-entity db framework-id) job-ents))))))

;;
;; /unscheduled_jobs
Expand Down