From 767374fec21e723cda6e7689840387a2fe8dc1bb Mon Sep 17 00:00:00 2001 From: Sarah Roberts Date: Tue, 30 Oct 2018 15:40:17 -0700 Subject: [PATCH 1/3] #129: mark job status updates for non-existent job steps as propagated if there are too many or too few matching job steps for processing --- src/apps/service/apps.clj | 10 ++++++---- src/apps/service/apps/jobs.clj | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/apps/service/apps.clj b/src/apps/service/apps.clj index d9f01b60..9c02c59e 100644 --- a/src/apps/service/apps.clj +++ b/src/apps/service/apps.clj @@ -220,16 +220,18 @@ (defn update-job-status ([external-id] (transaction - (let [{job-id :job_id} (jobs/get-unique-job-step external-id) - updates (jp/get-job-status-updates external-id)] + (let [updates (jp/get-job-status-updates external-id) + steps (jp/get-job-steps-by-external-id external-id)] (when (seq updates) - (let [job-step (jobs/lock-job-step job-id external-id) + (jobs/validate-job-status-update-step-count external-id updates steps) + (let [job-id (:job_id (first steps)) + job-step (jobs/lock-job-step job-id external-id) job (jobs/lock-job job-id) batch (when-let [parent-id (:parent_id job)] (jp/get-job-by-id parent-id)) apps-client (get-apps-client-for-username (:username job))] (doseq [{status :status sent-on :sent_on} updates] (let [end-date (when (jp/completed? status) (str sent-on)) - job-step (jobs/get-unique-job-step external-id)] + job-step (first (jp/get-job-steps-by-external-id external-id))] (when (jp/status-follows? status (:status job-step)) (jobs/update-job-status apps-client job-step job batch status end-date)))) (jp/mark-job-status-updates-propagated (mapv :id updates)) diff --git a/src/apps/service/apps/jobs.clj b/src/apps/service/apps/jobs.clj index acf1ce48..47afc1cb 100644 --- a/src/apps/service/apps/jobs.clj +++ b/src/apps/service/apps/jobs.clj @@ -14,16 +14,17 @@ [apps.service.apps.jobs.util :as ju] [apps.util.service :as service])) -(defn get-unique-job-step - "Gets a unique job step for an external ID. An exception is thrown if no job step - is found or if multiple job steps are found." - [external-id] - (let [job-steps (jp/get-job-steps-by-external-id external-id)] - (when (empty? job-steps) - (service/not-found "job step" external-id)) - (when (> (count job-steps) 1) - (service/not-unique "job step" external-id)) - (first job-steps))) +(defn validate-job-status-update-step-count + "Validates the number of steps for an external ID provided in a set of job status updates. If there are too many or + too few matching job steps then all of the job status updates will be marked as propagated and an exception will be + thrown." + [external-id updates job-steps] + (when (empty? job-steps) + (jp/mark-job-status-updates-propagated (mapv :id updates)) + (service/not-found "job step" external-id)) + (when (> (count job-steps) 1) + (jp/mark-job-status-updates-propagated (mapv :id updates)) + (service/not-unique "job step" external-id))) (defn lock-job-step [job-id external-id] From 409561bc407f44a1c13ce2b601869385bf06fc52 Mon Sep 17 00:00:00 2001 From: Sarah Roberts Date: Tue, 30 Oct 2018 16:29:30 -0700 Subject: [PATCH 2/3] #129: fixed a bug where the SQL statement to mark job status updates for non-existent jobs as propagated was being rolled back --- src/apps/persistence/jobs.clj | 10 +++++++++- src/apps/service/apps.clj | 7 +++---- src/apps/service/apps/jobs.clj | 17 +++++++++-------- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/apps/persistence/jobs.clj b/src/apps/persistence/jobs.clj index 32868776..ffffe357 100644 --- a/src/apps/persistence/jobs.clj +++ b/src/apps/persistence/jobs.clj @@ -787,6 +787,14 @@ (defn mark-job-status-updates-propagated "Marks a set of job status updates as propagated." [ids] + (when (seq ids) + (sql/update :job_status_updates + (set-fields {:propagated true}) + (where {:id [in ids]})))) + +(defn mark-job-status-updates-for-external-id-completed + "Marks all job status updates for an external ID as completed." + [external-id] (sql/update :job_status_updates (set-fields {:propagated true}) - (where {:id [in ids]}))) + (where {:external_id external-id}))) diff --git a/src/apps/service/apps.clj b/src/apps/service/apps.clj index 9c02c59e..d19b9e6c 100644 --- a/src/apps/service/apps.clj +++ b/src/apps/service/apps.clj @@ -219,12 +219,11 @@ (defn update-job-status ([external-id] + (jobs/validate-job-status-update-step-count external-id) (transaction - (let [updates (jp/get-job-status-updates external-id) - steps (jp/get-job-steps-by-external-id external-id)] + (let [updates (jp/get-job-status-updates external-id)] (when (seq updates) - (jobs/validate-job-status-update-step-count external-id updates steps) - (let [job-id (:job_id (first steps)) + (let [job-id (:job_id (jp/get-job-steps-by-external-id external-id)) job-step (jobs/lock-job-step job-id external-id) job (jobs/lock-job job-id) batch (when-let [parent-id (:parent_id job)] (jp/get-job-by-id parent-id)) diff --git a/src/apps/service/apps/jobs.clj b/src/apps/service/apps/jobs.clj index 47afc1cb..42ab37a7 100644 --- a/src/apps/service/apps/jobs.clj +++ b/src/apps/service/apps/jobs.clj @@ -17,14 +17,15 @@ (defn validate-job-status-update-step-count "Validates the number of steps for an external ID provided in a set of job status updates. If there are too many or too few matching job steps then all of the job status updates will be marked as propagated and an exception will be - thrown." - [external-id updates job-steps] - (when (empty? job-steps) - (jp/mark-job-status-updates-propagated (mapv :id updates)) - (service/not-found "job step" external-id)) - (when (> (count job-steps) 1) - (jp/mark-job-status-updates-propagated (mapv :id updates)) - (service/not-unique "job step" external-id))) + thrown. This function should not be called from within a transaction." + [external-id] + (let [job-steps (jp/get-job-steps-by-external-id external-id)] + (when (empty? job-steps) + (jp/mark-job-status-updates-for-external-id-completed external-id) + (service/not-found "job step" external-id)) + (when (> (count job-steps) 1) + (jp/mark-job-status-updates-for-external-id-completed external-id) + (service/not-unique "job step" external-id)))) (defn lock-job-step [job-id external-id] From f4c7c511b1edc17ec191a49b4e3fd794ba8e1bef Mon Sep 17 00:00:00 2001 From: Sarah Roberts Date: Thu, 1 Nov 2018 14:50:10 -0700 Subject: [PATCH 3/3] #129: fixed a bug --- src/apps/service/apps.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apps/service/apps.clj b/src/apps/service/apps.clj index d19b9e6c..2aead946 100644 --- a/src/apps/service/apps.clj +++ b/src/apps/service/apps.clj @@ -223,7 +223,7 @@ (transaction (let [updates (jp/get-job-status-updates external-id)] (when (seq updates) - (let [job-id (:job_id (jp/get-job-steps-by-external-id external-id)) + (let [job-id (:job_id (first (jp/get-job-steps-by-external-id external-id))) job-step (jobs/lock-job-step job-id external-id) job (jobs/lock-job job-id) batch (when-let [parent-id (:parent_id job)] (jp/get-job-by-id parent-id))