From 2d6b1c739f4aed6a3c51f1b663ff0d435862c4fe Mon Sep 17 00:00:00 2001 From: Jan Bernitt Date: Tue, 9 Jan 2024 12:54:21 +0100 Subject: [PATCH 1/2] fix: job scheduler delayed execution window handling and info [DHIS2-15027] --- .../dhis/scheduling/JobConfiguration.java | 43 +++++++++++++++-- .../dhis/scheduling/JobConfigurationTest.java | 12 +++-- .../DefaultJobConfigurationService.java | 9 ++-- .../HibernateJobConfigurationStore.java | 9 +++- .../hisp/dhis/scheduling/JobScheduler.java | 3 +- .../org/hisp/dhis/setting/SettingKey.java | 4 +- .../controller/scheduling/SchedulerEntry.java | 47 ++++++++++++++++++- 7 files changed, 109 insertions(+), 18 deletions(-) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/scheduling/JobConfiguration.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/scheduling/JobConfiguration.java index bda0caf76992..fa70ae8353f0 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/scheduling/JobConfiguration.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/scheduling/JobConfiguration.java @@ -36,6 +36,7 @@ import java.time.Duration; import java.time.Instant; import java.time.ZoneId; +import java.time.ZoneOffset; import java.time.temporal.ChronoUnit; import java.util.Date; import javax.annotation.CheckForNull; @@ -88,6 +89,14 @@ @ToString public class JobConfiguration extends BaseIdentifiableObject implements SecondaryMetadataObject { + /** + * A CRON based job may trigger on the same day after it has missed its execution. If time has + * passed past this point the execution is skipped, and it will trigger on the intended execution + * after that. This is the default value for the setting giving a 4h window to succeed with the + * execution for each occurrence. + */ + public static final int MAX_CRON_DELAY_HOURS = 4; + /** The type of job. */ @JsonProperty(required = true) private JobType jobType; @@ -309,10 +318,20 @@ public JobParameters getJobParameters() { /** Kept for backwards compatibility of the REST API */ @JsonProperty(access = JsonProperty.Access.READ_ONLY) public Date getNextExecutionTime() { - Instant next = nextExecutionTime(Instant.now(), Duration.ofDays(1)); + // this is a "best guess" because the 4h max delay could have been changed in the settings + Instant next = nextExecutionTime(Instant.now(), Duration.ofHours(MAX_CRON_DELAY_HOURS)); return next == null ? null : Date.from(next); } + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + public Date getMaxDelayedExecutionTime() { + Duration maxCronDelay = Duration.ofHours(MAX_CRON_DELAY_HOURS); + Instant nextExecutionTime = nextExecutionTime(Instant.now(), maxCronDelay); + if (nextExecutionTime == null) return null; + Instant instant = maxDelayedExecutionTime(this, maxCronDelay, nextExecutionTime); + return instant == null ? null : Date.from(instant); + } + /** Kept for backwards compatibility of the REST API */ @JsonProperty(access = JsonProperty.Access.READ_ONLY) public String getLastRuntimeExecution() { @@ -375,23 +394,29 @@ public Instant nextExecutionTime(@Nonnull Instant now, @Nonnull Duration maxCron Instant nextExecutionTime( @Nonnull ZoneId zone, @Nonnull Instant now, @Nonnull Duration maxCronDelay) { // for good measure we offset the last time by 1 second - Instant since = lastExecuted == null ? now : lastExecuted.toInstant().plusSeconds(1); + boolean isFirstExecution = lastExecuted == null; + Instant since = isFirstExecution ? now : lastExecuted.toInstant().plusSeconds(1); if (isUsedInQueue() && getQueuePosition() > 0) return null; return switch (getSchedulingType()) { case ONCE_ASAP -> nextOnceExecutionTime(since); case FIXED_DELAY -> nextDelayExecutionTime(since); - case CRON -> nextCronExecutionTime(zone, since, now, maxCronDelay); + case CRON -> nextCronExecutionTime( + zone, isFirstExecution ? since.minus(maxCronDelay) : since, now, maxCronDelay); }; } private Instant nextCronExecutionTime( @Nonnull ZoneId zone, @Nonnull Instant since, Instant now, @Nonnull Duration maxDelay) { if (isUndefinedCronExpression(cronExpression)) return null; - SimpleTriggerContext context = new SimpleTriggerContext(Clock.fixed(since, zone)); + // we use a no offset zone for the context as we want the given since value to be taken as is + // the zone is not actually used by this is closest to what would be required + ZoneOffset noOffsetZone = ZoneOffset.UTC; + SimpleTriggerContext context = new SimpleTriggerContext(Clock.fixed(since, noOffsetZone)); Date next = new CronTrigger(cronExpression, zone).nextExecutionTime(context); if (next == null) return null; while (next != null && now.isAfter(next.toInstant().plus(maxDelay))) { - context = new SimpleTriggerContext(Clock.fixed(next.toInstant().plusSeconds(1), zone)); + context = + new SimpleTriggerContext(Clock.fixed(next.toInstant().plusSeconds(1), noOffsetZone)); next = new CronTrigger(cronExpression, zone).nextExecutionTime(context); } return next == null ? null : next.toInstant(); @@ -409,6 +434,14 @@ private Instant nextOnceExecutionTime(@Nonnull Instant since) { return since; } + @CheckForNull + public static Instant maxDelayedExecutionTime( + JobConfiguration config, Duration maxCronDelay, Instant nextExecutionTime) { + return nextExecutionTime == null || config.getSchedulingType() != SchedulingType.CRON + ? null + : nextExecutionTime.plus(maxCronDelay); + } + private static boolean isUndefinedCronExpression(String cronExpression) { return cronExpression == null || cronExpression.isEmpty() diff --git a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/scheduling/JobConfigurationTest.java b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/scheduling/JobConfigurationTest.java index f8fc517e6987..142796210846 100644 --- a/dhis-2/dhis-api/src/test/java/org/hisp/dhis/scheduling/JobConfigurationTest.java +++ b/dhis-2/dhis-api/src/test/java/org/hisp/dhis/scheduling/JobConfigurationTest.java @@ -142,12 +142,18 @@ void cronNextExecutionTime_MaxCronDelay() { ZonedDateTime today10am = todayMidnight.withHour(10); ZonedDateTime tomorrow8_40am = today8_40am.plusDays(1); - // when the job never executed the next execution is on the next day the intended time - // if now is already after the intended time + // when the job never executed the next execution is today's as long as + // now is in the window from intended execution time to max delayed execution time assertEquals( - tomorrow8_40am.toInstant(), + today8_40am.toInstant(), config.nextExecutionTime(zone, today10am.toInstant(), maxCronDelay)); + // when the job never executed the next execution is tomorrow when + // now is after the window from intended execution time to max delayed execution time + assertEquals( + tomorrow8_40am.toInstant(), + config.nextExecutionTime(zone, today10am.plusHours(5).toInstant(), maxCronDelay)); + // when the job did execute last yesterday the intended time, // and we are still in the 2h window after 8:40am at 10am // the job still wants to run today 8:40am (immediately as that time has passed) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java index 8f46b2a4eb4d..e8682a27ab30 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/DefaultJobConfigurationService.java @@ -240,11 +240,10 @@ public List getDueJobConfigurations( Instant endOfWindow = now.plusSeconds(dueInNextSeconds); Duration maxCronDelay = Duration.ofHours(systemSettings.getIntSetting(SettingKey.JOBS_MAX_CRON_DELAY_HOURS)); - Stream dueJobs = - jobConfigurationStore - .getDueJobConfigurations(includeWaiting) - .filter(c -> c.isDueBetween(now, endOfWindow, maxCronDelay)); - return dueJobs.toList(); + return jobConfigurationStore + .getDueJobConfigurations(includeWaiting) + .filter(c -> c.isDueBetween(now, endOfWindow, maxCronDelay)) + .toList(); } @Override diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HibernateJobConfigurationStore.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HibernateJobConfigurationStore.java index 611c568b8975..fe3800c651a7 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HibernateJobConfigurationStore.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HibernateJobConfigurationStore.java @@ -335,6 +335,7 @@ public boolean tryStart(@Nonnull String jobId) { """ update jobconfiguration j1 set + lastupdated = now(), jobstatus = 'RUNNING', lastexecuted = now(), lastalive = now(), @@ -360,6 +361,7 @@ public boolean tryCancel(@Nonnull String jobId) { """ update jobconfiguration set + lastupdated = now(), cancel = case when jobstatus = 'RUNNING' then true else false end, enabled = case when queueposition is null and schedulingtype = 'ONCE_ASAP' and cronexpression is null and delay is null then false @@ -389,6 +391,7 @@ public boolean tryFinish(@Nonnull String jobId, JobStatus status) { """ update jobconfiguration set + lastupdated = now(), lastexecutedstatus = case when cancel = true then 'STOPPED' else :status end, lastfinished = now(), lastalive = null, @@ -421,6 +424,7 @@ public boolean trySkip(@Nonnull String queue) { """ update jobconfiguration set + lastupdated = now(), lastexecutedstatus = 'NOT_STARTED', lastexecuted = now(), lastfinished = now(), @@ -463,7 +467,9 @@ public int updateDisabledJobs() { String sql = """ update jobconfiguration - set jobstatus = 'DISABLED' + set + lastupdated = now(), + jobstatus = 'DISABLED' where jobstatus = 'SCHEDULED' and enabled = false """; @@ -505,6 +511,7 @@ public int rescheduleStaleJobs(int timeoutMinutes) { """ update jobconfiguration set + lastupdated = now(), jobstatus = 'SCHEDULED', cancel = false, lastexecutedstatus = 'FAILED', diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/JobScheduler.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/JobScheduler.java index 21db10c661f3..9bbd1677b340 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/JobScheduler.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/JobScheduler.java @@ -99,7 +99,7 @@ public class JobScheduler implements Runnable, JobRunner { public void start() { long loopTimeMs = LOOP_SECONDS * 1000L; - long alignment = currentTimeMillis() % loopTimeMs; + long alignment = loopTimeMs - (currentTimeMillis() % loopTimeMs); Executors.newSingleThreadScheduledExecutor() .scheduleAtFixedRate(this, alignment, loopTimeMs, TimeUnit.MILLISECONDS); scheduling.set(true); @@ -221,6 +221,7 @@ private void runDueJob(JobConfiguration config, Instant start) { jobId, start.atZone(ZoneId.systemDefault()))); return; } + log.debug("Running job %s"); JobProgress progress = null; try { AtomicLong lastAlive = new AtomicLong(currentTimeMillis()); diff --git a/dhis-2/dhis-services/dhis-service-setting/src/main/java/org/hisp/dhis/setting/SettingKey.java b/dhis-2/dhis-services/dhis-service-setting/src/main/java/org/hisp/dhis/setting/SettingKey.java index 854bae241956..e6f7b0ca9035 100644 --- a/dhis-2/dhis-services/dhis-service-setting/src/main/java/org/hisp/dhis/setting/SettingKey.java +++ b/dhis-2/dhis-services/dhis-service-setting/src/main/java/org/hisp/dhis/setting/SettingKey.java @@ -50,6 +50,7 @@ import org.hisp.dhis.fileresource.FileResourceRetentionStrategy; import org.hisp.dhis.i18n.locale.LocaleManager; import org.hisp.dhis.period.RelativePeriodEnum; +import org.hisp.dhis.scheduling.JobConfiguration; import org.hisp.dhis.sms.config.SmsConfiguration; /** @@ -253,7 +254,8 @@ public enum SettingKey { * its intended time of the day to trigger. If time has passed past this point the execution for * that day is skipped, and it will trigger on the intended time the day after. */ - JOBS_MAX_CRON_DELAY_HOURS("jobsMaxCronDelayHours", 4, Integer.class), + JOBS_MAX_CRON_DELAY_HOURS( + "jobsMaxCronDelayHours", JobConfiguration.MAX_CRON_DELAY_HOURS, Integer.class), /** * A job running with a smaller delay than the given value is logged on debug level instead of diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/scheduling/SchedulerEntry.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/scheduling/SchedulerEntry.java index f2659ec593c4..a39f5a502866 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/scheduling/SchedulerEntry.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/scheduling/SchedulerEntry.java @@ -29,12 +29,14 @@ import static java.util.Comparator.comparing; import static java.util.stream.Collectors.toList; +import static org.hisp.dhis.scheduling.JobConfiguration.maxDelayedExecutionTime; import com.fasterxml.jackson.annotation.JsonProperty; import java.time.Duration; import java.time.Instant; import java.util.Date; import java.util.List; +import javax.annotation.CheckForNull; import lombok.Value; import org.hisp.dhis.scheduling.JobConfiguration; import org.hisp.dhis.scheduling.JobStatus; @@ -51,6 +53,27 @@ class SchedulerEntry { @JsonProperty Date nextExecutionTime; + /** + * The end of the window for the current {@link #nextExecutionTime}. This means if execution is + * missed on the intended time this will be the latest time an attempt is made for that occurrence + * before the next time will be the occurrence after that. + */ + @JsonProperty Date maxDelayedExecutionTime; + + /** + * The number of seconds until the next execution will happen. This is purely for convenience to + * spare the client to fetch the server time to compute this value as the client can not use the + * client local time to compute it from {@link #nextExecutionTime} + */ + @JsonProperty Long secondsToNextExecutionTime; + + /** + * The number of seconds until the end of the execution window. This is purely for convenience to + * * spare the client to fetch the server time to compute this value as the client can not use the + * * client local time to compute it from {@link #maxDelayedExecutionTime} + */ + @JsonProperty Long secondsToMaxDelayedExecutionTime; + @JsonProperty JobStatus status; @JsonProperty boolean enabled; @@ -61,12 +84,17 @@ class SchedulerEntry { static SchedulerEntry of(JobConfiguration config, Duration maxCronDelay) { Instant nextExecutionTime = config.nextExecutionTime(Instant.now(), maxCronDelay); + Instant maxDelayedExecutionTime = + maxDelayedExecutionTime(config, maxCronDelay, nextExecutionTime); return new SchedulerEntry( config.getName(), config.getJobType().name(), config.getCronExpression(), config.getDelay(), - nextExecutionTime == null ? null : Date.from(nextExecutionTime), + dateOf(nextExecutionTime), + dateOf(maxDelayedExecutionTime), + secondsUntil(nextExecutionTime), + secondsUntil(maxDelayedExecutionTime), config.getJobStatus(), config.isEnabled(), config.getJobType().isUserDefined(), @@ -87,12 +115,17 @@ static SchedulerEntry of(List jobs, Duration maxCronDelay) { .findAny() .orElse(trigger.getJobStatus()); Instant nextExecutionTime = trigger.nextExecutionTime(Instant.now(), maxCronDelay); + Instant maxDelayedExecutionTime = + maxDelayedExecutionTime(trigger, maxCronDelay, nextExecutionTime); return new SchedulerEntry( trigger.getQueueName(), "Sequence", trigger.getCronExpression(), trigger.getDelay(), - nextExecutionTime == null ? null : Date.from(nextExecutionTime), + dateOf(nextExecutionTime), + dateOf(maxDelayedExecutionTime), + secondsUntil(nextExecutionTime), + secondsUntil(maxDelayedExecutionTime), queueStatus, trigger.isEnabled(), true, @@ -101,4 +134,14 @@ static SchedulerEntry of(List jobs, Duration maxCronDelay) { .map(SchedulerEntryJob::of) .collect(toList())); } + + private static Date dateOf(Instant instant) { + return instant == null ? null : Date.from(instant); + } + + @CheckForNull + public static Long secondsUntil(Instant instant) { + if (instant == null) return null; + return Duration.between(Instant.now(), instant).getSeconds(); + } } From 625b37c816823f86ecce98b8bcb7ea367b030c5d Mon Sep 17 00:00:00 2001 From: Jan Bernitt Date: Tue, 9 Jan 2024 17:58:54 +0100 Subject: [PATCH 2/2] chore: rename HousekeepingJob --- .../scheduling/{HeartbeatJob.java => HousekeepingJob.java} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/{HeartbeatJob.java => HousekeepingJob.java} (97%) diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HeartbeatJob.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HousekeepingJob.java similarity index 97% rename from dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HeartbeatJob.java rename to dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HousekeepingJob.java index ec3830ada9ee..657700c7126a 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HeartbeatJob.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/scheduling/HousekeepingJob.java @@ -47,7 +47,7 @@ */ @Component @RequiredArgsConstructor -public class HeartbeatJob implements Job { +public class HousekeepingJob implements Job { private final JobSchedulerLoopService jobSchedulerService; private final JobConfigurationService jobConfigurationService; @@ -59,7 +59,7 @@ public JobType getJobType() { @Override public void execute(JobConfiguration config, JobProgress progress) { - progress.startingProcess("Heartbeat"); + progress.startingProcess("Housekeeping"); progress.startingStage("Apply job cancellation", SKIP_STAGE); progress.runStage(