From 281bba823ea010902f066042922a25ce8227c2d2 Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Wed, 25 Sep 2024 11:09:03 +0100 Subject: [PATCH 1/2] Avoid unnecessarily reregistering Segue Jobs Currently we reregister jobs on every API start, even if they are unchanged from the version existing in the database. (The code of the job can change without the _job_ changing; the job records the class to execute and the cron record). Doing so overwrites the last run time of the job, which prevents us from doing sensible things if the API has been offline for a while. This change only reregisters the job if the job details or cron string change. --- .../dtg/segue/scheduler/SegueJobService.java | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java index 4e8bb203bf..77265fefdd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java @@ -16,9 +16,6 @@ package uk.ac.cam.cl.dtg.segue.scheduler; import com.google.inject.Inject; -import jakarta.annotation.Nullable; -import jakarta.servlet.ServletContextEvent; -import jakarta.servlet.ServletContextListener; import org.quartz.CronTrigger; import org.quartz.JobBuilder; import org.quartz.JobDataMap; @@ -31,6 +28,9 @@ import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; +import jakarta.annotation.Nullable; +import jakarta.servlet.ServletContextEvent; +import jakarta.servlet.ServletContextListener; import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; @@ -58,7 +58,7 @@ public class SegueJobService implements ServletContextListener { public SegueJobService(final PostgresSqlDb database, final List allKnownJobs, @Nullable final List jobsToRemove) { this.allKnownJobs = allKnownJobs; - this.jobsToRemove =jobsToRemove; + this.jobsToRemove = jobsToRemove; this.localRegisteredJobs = new ArrayList<>(); StdSchedulerFactory stdSchedulerFactory = new StdSchedulerFactory(); @@ -107,9 +107,9 @@ public void removeScheduleJob(final SegueScheduledJob jobToRemove) throws Schedu localRegisteredJobs.remove(jobToRemove); if (deletionNeeded) { - log.info(String.format("Removed existing job: %s", jobToRemove.getJobKey())); + log.info("Removed existing job: {}", jobToRemove.getJobKey()); } else { - log.info(String.format("Skipping removal of non-registered job: %s", jobToRemove.getJobKey())); + log.info("Skipping removal of non-registered job: {}", jobToRemove.getJobKey()); } } @@ -142,13 +142,18 @@ public void registerScheduleJob(final SegueScheduledJob jobToRegister) throws Sc scheduler.getContext().put(jobToRegister.getExecutableTask().getClass().getName(), jobToRegister.getExecutionContext()); - if (!scheduler.checkExists(job.getKey())) { + if (!scheduler.checkExists(job.getKey()) || !scheduler.checkExists(cronTrigger.getKey())) { scheduler.scheduleJob(job, cronTrigger); - log.info(String.format("Registered job (%s) to segue job execution service. Current jobs registered (%s): ", jobToRegister.getJobKey(), localRegisteredJobs.size())); + log.info("Registered Quartz job ({}). Current jobs registered ({}): ", jobToRegister.getJobKey(), localRegisteredJobs.size()); } else { - // FIXME - this does not update the job details, e.g. if the SQL file name changes. - scheduler.rescheduleJob(cronTrigger.getKey(), cronTrigger); - log.info(String.format("Re-registered job (%s) to segue job execution service. Current jobs registered (%s): ", jobToRegister.getJobKey(), localRegisteredJobs.size())); + CronTrigger existingTrigger = (CronTrigger) scheduler.getTrigger(cronTrigger.getKey()); + if (!existingTrigger.equals(cronTrigger)) { + // FIXME - this does not update the job details, e.g. if the SQL file name changes. + scheduler.rescheduleJob(cronTrigger.getKey(), cronTrigger); + log.info("Re-registered Quartz job ({}). Current jobs registered ({}): ", jobToRegister.getJobKey(), localRegisteredJobs.size()); + } else { + log.info("Skipping re-registering existing Quartz job ({}). Current jobs registered ({}): ", jobToRegister.getJobKey(), localRegisteredJobs.size()); + } } localRegisteredJobs.add(jobToRegister); From 89558e191cd7fbfe652c45cdfc2b4ee5e4dbfe79 Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Tue, 1 Oct 2024 17:15:08 +0100 Subject: [PATCH 2/2] Correctly reregister job on cron expression change Comparing the CronTriggers for equality only checked that the keys were the same, which was true by definition since we loaded the the existing trigger using the key of the current trigger. This was oversight on my part! --- .../java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java index 77265fefdd..ef2fc4b8dd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/SegueJobService.java @@ -35,6 +35,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Objects; import static org.quartz.CronScheduleBuilder.cronSchedule; @@ -147,7 +148,7 @@ public void registerScheduleJob(final SegueScheduledJob jobToRegister) throws Sc log.info("Registered Quartz job ({}). Current jobs registered ({}): ", jobToRegister.getJobKey(), localRegisteredJobs.size()); } else { CronTrigger existingTrigger = (CronTrigger) scheduler.getTrigger(cronTrigger.getKey()); - if (!existingTrigger.equals(cronTrigger)) { + if (!Objects.equals(existingTrigger.getCronExpression(), cronTrigger.getCronExpression())) { // FIXME - this does not update the job details, e.g. if the SQL file name changes. scheduler.rescheduleJob(cronTrigger.getKey(), cronTrigger); log.info("Re-registered Quartz job ({}). Current jobs registered ({}): ", jobToRegister.getJobKey(), localRegisteredJobs.size());