Adds support for including custom executor jobs in /jobs #756

dposada · 2018-03-09T04:20:21Z

Changes proposed in this PR

adding the ability to list jobs on /jobs
/jobs will not filter out custom executor jobs

Why are we making these changes?

We want to give users and tools the option of including custom executor jobs when listing (while maintaining backwards compatibility).

DaoWen

Are you planning to update the /list endpoint later?

DaoWen · 2018-03-09T07:33:46Z

scheduler/src/cook/mesos/api.clj

@@ -2041,7 +2040,8 @@
                                ::start-ms (parse-long-default start-ms nil)
                                ::limit (parse-int-default limit Integer/MAX_VALUE)
                                ::end-ms (parse-long-default end-ms (System/currentTimeMillis))
-                                ::name-filter-fn (name-filter-str->name-filter-fn name)}]
+                                ::name-filter-fn (name-filter-str->name-filter-fn name)
+                                ::include-custom-executor? (Boolean/parseBoolean include-custom-executor)}]


Nitpick: It's probably better to use Boolean/valueOf here so as to avoid the implicit auto-boxing. (valueOf returns a Boolean object, whereas parseBoolean returns a boolean primitive).

DaoWen · 2018-03-09T07:48:21Z

scheduler/src/cook/mesos/util.clj

+                  (take-while #(and (< (:e %) entid-end)
+                                    (= (:a %) job-user-entid)
+                                    (= (:v %) user)))
+                  (map #(:e %))


η-reduction! (map #(:e %)) → (map :e)

DaoWen · 2018-03-09T07:51:46Z

scheduler/src/cook/mesos/util.clj

+                  (filter #(< (.getTime (:job/submit-time %)) (.getTime end)))
+                  (filter #(= state-keyword (:job/state %))))]
+    (cond->> jobs
+             (not include-custom-executor?) (filter #(not (:job/custom-executor %))))))


I know this was carried over from the old code, but can we change the (filter #(not (:job/custom-executor %)) to (remove :job/custom-executor)?

dposada · 2018-03-09T15:11:24Z

@DaoWen

Are you planning to update the /list endpoint later?

I don't understand the question -- this PR does update the /list endpoint

shamsimam · 2018-03-09T16:13:31Z

scheduler/src/cook/mesos/api.clj

@@ -2015,8 +2015,7 @@
    :malformed? (fn [ctx]
                  ;; since-hours-ago is included for backwards compatibility but is deprecated
                  ;; please use start-ms and end-ms instead
-                  (let [{:keys [state user since-hours-ago start-ms end-ms limit name]
-                         :as params}
+                  (let [{:keys [state user since-hours-ago start-ms end-ms limit name include-custom-executor]}


Nitpick: lexicographic order

DaoWen · 2018-03-09T17:07:46Z

I didn't see any swagger updates, so I assumed you hadn't touched the actual endpoint. I didn't realize that it doesn't have any swagger annotations (#307). 😢

I guess that's another thing to fix whenever we get around to fixing our outdated swagger docs.

shamsimam

Completed my pass, added some comments.

shamsimam · 2018-03-14T18:05:36Z

scheduler/src/cook/mesos/api.clj

+    (set/difference states util/instance-states)
+    (if (set/superset? states util/instance-states)
+      (-> states (set/difference util/instance-states) (conj "completed"))
+      states)))


This might be easier to read:

(def terminal states #{"completed" "failed" "success"}) ... (if (some states terminal-states) (-> states (set/difference terminal-states) (conj "completed")) states)

This is the current (and desired) behavior:

(api/normalize-list-states #{"success" "failed" "completed"}) => #{"completed"} (api/normalize-list-states #{"success" "failed"}) => #{"completed"} (api/normalize-list-states #{"success"}) => #{"success"} (api/normalize-list-states #{"failed"}) => #{"failed"} (api/normalize-list-states #{"completed"}) => #{"completed"}

I don't think your suggestion will have the same results when states is #{"success"}, for example.

shamsimam · 2018-03-14T18:06:23Z

scheduler/src/cook/mesos/api.clj

+      (-> states (set/difference util/instance-states) (conj "completed"))
+      states)))
+
+(defn parse-time


Does this belong in util?

shamsimam · 2018-03-14T18:06:28Z

scheduler/src/cook/mesos/api.clj

+  (or (tf/parse s)
+      (tc/from-long (Long/parseLong s))))
+
+(defn- parse-int-default


Does this belong in util?

shamsimam · 2018-03-14T18:08:23Z

scheduler/src/cook/mesos/api.clj

+    (let [pattern (name-filter-str->name-filter-pattern name)]
+      #(re-matches pattern %))))
+
+(defn job-list-request-malformed?


Missing docstring

Are start and end required parameters?

shamsimam · 2018-03-14T18:23:59Z

scheduler/test/cook/test/mesos/api.clj

+    (is (= 201 (:status response-1)))
+    (is (= 2 (count jobs)))
+    (is (= (:uuid response-2) (-> jobs first (get "uuid"))))
+    (is (= (:uuid response-1) (-> jobs second (get "uuid"))))))


Should we assert on the :job/custom-executor flags of the two jobs also?

sradack · 2018-03-16T20:47:39Z

scheduler/src/cook/mesos/util.clj

+                  (filter #(< (.getTime (:job/submit-time %)) (.getTime end)))
+                  (filter #(= state-keyword (:job/state %))))]
+    (cond->> jobs
+             (not include-custom-executor?) (remove :job/custom-executor))))


If include custom-exector jobs, do you need to check the commit-latch to avoid returning uncommitted jobs?

Good call, this is done now

…-custom-executor

dposada · 2018-03-21T17:12:23Z

@shamsimam @sradack Any additional feedback on this?

dposada · 2018-03-21T17:38:06Z

Can we merge?

* Adds support for including custom executor jobs in /list * Fixes broken unit tests * Addresses feedback from code review * Adds support for listing jobs with /jobs * Feedback from code review * Removes uncommitted jobs from results * Fixes the filtering for non-custom-executor jobs * Marks test_kill_task_terminate_with_sigterm as xfail * Adds missing import

Adds support for including custom executor jobs in /list

b4a82db

dposada added enhancement wip labels Mar 9, 2018

dposada self-assigned this Mar 9, 2018

Fixes broken unit tests

87dbdc4

dposada removed the wip label Mar 9, 2018

dposada requested a review from shamsimam March 9, 2018 04:55

DaoWen approved these changes Mar 9, 2018

View reviewed changes

Addresses feedback from code review

ecaf5e7

shamsimam reviewed Mar 9, 2018

View reviewed changes

Adds support for listing jobs with /jobs

1d93ebb

dposada changed the title ~~Adds support for including custom executor jobs in /list~~ Adds support for including custom executor jobs in /jobs Mar 12, 2018

shamsimam reviewed Mar 14, 2018

View reviewed changes

dposada and others added 2 commits March 17, 2018 22:22

Merge branch 'master' into list-custom-executor

6fb5876

Feedback from code review

208ef58

sradack reviewed Mar 18, 2018

View reviewed changes

Daniel Posada and others added 6 commits March 19, 2018 13:29

Removes uncommitted jobs from results

74a151b

Fixes the filtering for non-custom-executor jobs

37dcb10

Merge branch 'master' into list-custom-executor

21e2832

Marks test_kill_task_terminate_with_sigterm as xfail

931d9fa

Merge remote-tracking branch 'dposada/list-custom-executor' into list…

8a1be3e

…-custom-executor

Adds missing import

8ee6ee8

shamsimam approved these changes Mar 21, 2018

View reviewed changes

sradack approved these changes Mar 21, 2018

View reviewed changes

shamsimam merged commit d236ec3 into twosigma:master Mar 21, 2018

scrosby mentioned this pull request May 31, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for including custom executor jobs in /jobs #756

Adds support for including custom executor jobs in /jobs #756

dposada commented Mar 9, 2018 •

edited

Loading

DaoWen left a comment

DaoWen Mar 9, 2018

dposada Mar 9, 2018

DaoWen Mar 9, 2018

dposada Mar 9, 2018

DaoWen Mar 9, 2018

dposada Mar 9, 2018

dposada commented Mar 9, 2018

shamsimam Mar 9, 2018

DaoWen commented Mar 9, 2018

shamsimam left a comment

shamsimam Mar 14, 2018

dposada Mar 18, 2018

shamsimam Mar 21, 2018

shamsimam Mar 14, 2018

dposada Mar 18, 2018

shamsimam Mar 14, 2018

dposada Mar 18, 2018

shamsimam Mar 14, 2018

shamsimam Mar 14, 2018

dposada Mar 18, 2018

shamsimam Mar 14, 2018

dposada Mar 18, 2018

sradack Mar 16, 2018

dposada Mar 21, 2018

dposada commented Mar 21, 2018

dposada commented Mar 21, 2018

Adds support for including custom executor jobs in /jobs #756

Adds support for including custom executor jobs in /jobs #756

Conversation

dposada commented Mar 9, 2018 • edited Loading

Changes proposed in this PR

Why are we making these changes?

DaoWen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dposada commented Mar 9, 2018

Choose a reason for hiding this comment

DaoWen commented Mar 9, 2018

shamsimam left a 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

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

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

Choose a reason for hiding this comment

dposada commented Mar 21, 2018

dposada commented Mar 21, 2018

dposada commented Mar 9, 2018 •

edited

Loading