Repository: aurora Updated Branches: refs/heads/master edaf984ad -> 1c7343831
Fixup `getJobSummary` for cron jobs with invalid next run dates. Previously, the fact `null` could be returned by Quartz when calculating the next run was not taken into account. Now The CronPredictor interface makes this possibility manifest with an Optional result. Code that uses the CronPredictor is adjusted and tests are added. NB: This code is as first proposed here by Brice Arnould with small changes: https://reviews.apache.org/r/39170/ Testing Done: Green locally: `./gradlew -Pq build` Bugs closed: AURORA-1385 Reviewed at https://reviews.apache.org/r/41528/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/1c734383 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/1c734383 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/1c734383 Branch: refs/heads/master Commit: 1c7343831b4131d89d8aeea3c730b31e171326cb Parents: edaf984 Author: John Sirois <[email protected]> Authored: Fri Dec 18 12:28:45 2015 -0500 Committer: Zameer Manji <[email protected]> Committed: Fri Dec 18 12:28:45 2015 -0500 ---------------------------------------------------------------------- .../aurora/scheduler/cron/CronPredictor.java | 9 +++-- .../cron/quartz/CronPredictorImpl.java | 8 +++-- .../scheduler/thrift/ReadOnlySchedulerImpl.java | 13 +++++--- .../cron/quartz/CronPredictorImplTest.java | 23 ++++++++++--- .../thrift/ReadOnlySchedulerImplTest.java | 35 ++++++++++++++++++-- 5 files changed, 71 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/1c734383/src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java b/src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java index 043ba7e..8957b3a 100644 --- a/src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java +++ b/src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java @@ -15,15 +15,20 @@ package org.apache.aurora.scheduler.cron; import java.util.Date; +import com.google.common.base.Optional; + /** * A utility function that predicts a cron run given a schedule. */ public interface CronPredictor { /** * Predicts the next date at which a cron schedule will trigger. + * <p> + * NB: Some cron schedules can predict a run at an invalid date (eg: too far in the future); and + * it's these predictions that will result in an absent result. * * @param schedule Cron schedule to predict the next time for. - * @return A prediction for the next time a cron will run. + * @return A prediction for the next time a cron will run if a valid prediction can be made. */ - Date predictNextRun(CrontabEntry schedule); + Optional<Date> predictNextRun(CrontabEntry schedule); } http://git-wip-us.apache.org/repos/asf/aurora/blob/1c734383/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java b/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java index 1ddc4e1..e937667 100644 --- a/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java +++ b/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java @@ -18,8 +18,9 @@ import java.util.TimeZone; import javax.inject.Inject; -import org.apache.aurora.common.util.Clock; +import com.google.common.base.Optional; +import org.apache.aurora.common.util.Clock; import org.apache.aurora.scheduler.cron.CronPredictor; import org.apache.aurora.scheduler.cron.CrontabEntry; import org.quartz.CronExpression; @@ -37,8 +38,9 @@ class CronPredictorImpl implements CronPredictor { } @Override - public Date predictNextRun(CrontabEntry schedule) { + public Optional<Date> predictNextRun(CrontabEntry schedule) { CronExpression cronExpression = Quartz.cronExpression(schedule, timeZone); - return cronExpression.getNextValidTimeAfter(new Date(clock.nowMillis())); + // The getNextValidTimeAfter call may return null; eg: if the date is too far in the future. + return Optional.fromNullable(cronExpression.getNextValidTimeAfter(new Date(clock.nowMillis()))); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/1c734383/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java b/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java index c0e8a20..9f0cf86 100644 --- a/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java +++ b/src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java @@ -14,6 +14,7 @@ package org.apache.aurora.scheduler.thrift; import java.util.Collection; +import java.util.Date; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -29,7 +30,6 @@ import com.google.common.base.Joiner; import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.base.Predicates; -import com.google.common.base.Strings; import com.google.common.collect.FluentIterable; import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableSet; @@ -247,10 +247,13 @@ class ReadOnlySchedulerImpl implements ReadOnlyScheduler.Iface { .setJob(job.newBuilder()) .setStats(Jobs.getJobStats(tasks.get(jobKey)).newBuilder()); - return Strings.isNullOrEmpty(job.getCronSchedule()) - ? summary - : summary.setNextCronRunMs( - cronPredictor.predictNextRun(CrontabEntry.parse(job.getCronSchedule())).getTime()); + if (job.isSetCronSchedule()) { + CrontabEntry crontabEntry = CrontabEntry.parse(job.getCronSchedule()); + Optional<Date> nextRun = cronPredictor.predictNextRun(crontabEntry); + return nextRun.transform(date -> summary.setNextCronRunMs(date.getTime())).or(summary); + } else { + return summary; + } }; ImmutableSet<JobSummary> jobSummaries = http://git-wip-us.apache.org/repos/asf/aurora/blob/1c734383/src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java b/src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java index b85f4ab..0f8c983 100644 --- a/src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java @@ -17,6 +17,7 @@ import java.util.Date; import java.util.List; import java.util.TimeZone; +import com.google.common.base.Optional; import com.google.common.collect.Lists; import org.apache.aurora.common.quantity.Amount; @@ -29,6 +30,7 @@ import org.junit.Before; import org.junit.Test; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; public class CronPredictorImplTest { private static final TimeZone TIME_ZONE = TimeZone.getTimeZone("GMT"); @@ -49,7 +51,9 @@ public class CronPredictorImplTest { clock.advance(Amount.of(1L, Time.DAYS)); Date expectedPrediction = new Date(Amount.of(1L, Time.DAYS).as(Time.MILLISECONDS) + Amount.of(1L, Time.MINUTES).as(Time.MILLISECONDS)); - assertEquals(expectedPrediction, cronPredictor.predictNextRun(CrontabEntry.parse("* * * * *"))); + assertEquals( + Optional.of(expectedPrediction), + cronPredictor.predictNextRun(CrontabEntry.parse("* * * * *"))); } @Test @@ -59,14 +63,25 @@ public class CronPredictorImplTest { } @Test + public void testInvalidPrediction() { + // Too far in the future to represent as a Date. + clock.advance(Amount.of(Long.MAX_VALUE, Time.DAYS)); + assertEquals(Optional.absent(), cronPredictor.predictNextRun(CrontabEntry.parse("* * * * *"))); + } + + @Test public void testCronPredictorConforms() throws Exception { for (ExpectedPrediction expectedPrediction : ExpectedPrediction.getAll()) { List<Date> results = Lists.newArrayList(); clock.setNowMillis(0); for (int i = 0; i < expectedPrediction.getTriggerTimes().size(); i++) { - Date nextTriggerTime = cronPredictor.predictNextRun(expectedPrediction.parseCrontabEntry()); - results.add(nextTriggerTime); - clock.setNowMillis(nextTriggerTime.getTime()); + Optional<Date> nextTriggerTime = + cronPredictor.predictNextRun(expectedPrediction.parseCrontabEntry()); + assertTrue(nextTriggerTime.isPresent()); + + Date triggerTime = nextTriggerTime.get(); + results.add(triggerTime); + clock.setNowMillis(triggerTime.getTime()); } assertEquals( "Cron schedule " + expectedPrediction.getSchedule() + " made unexpected predictions.", http://git-wip-us.apache.org/repos/asf/aurora/blob/1c734383/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java index 6efe03f..77d5c1d 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java @@ -40,6 +40,7 @@ import org.apache.aurora.gen.JobConfiguration; import org.apache.aurora.gen.JobKey; import org.apache.aurora.gen.JobStats; import org.apache.aurora.gen.JobSummary; +import org.apache.aurora.gen.JobSummaryResult; import org.apache.aurora.gen.JobUpdate; import org.apache.aurora.gen.JobUpdateDetails; import org.apache.aurora.gen.JobUpdateKey; @@ -114,6 +115,7 @@ import static org.apache.aurora.scheduler.thrift.Fixtures.nonProductionTask; import static org.apache.aurora.scheduler.thrift.ReadOnlySchedulerImpl.NO_CRON; import static org.easymock.EasyMock.expect; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; public class ReadOnlySchedulerImplTest extends EasyMockTest { @@ -190,11 +192,11 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { .setTaskConfig(ownedImmediateTaskInfo); Builder query = Query.roleScoped(ROLE); - Set<JobSummary> ownedImmedieteJobSummaryOnly = ImmutableSet.of( + Set<JobSummary> ownedImmediateJobSummaryOnly = ImmutableSet.of( new JobSummary().setJob(ownedImmediateJob).setStats(new JobStats().setActiveTaskCount(1))); expect(cronPredictor.predictNextRun(CrontabEntry.parse(CRON_SCHEDULE))) - .andReturn(new Date(nextCronRunMs)) + .andReturn(Optional.of(new Date(nextCronRunMs))) .anyTimes(); storageUtil.expectTaskFetch(query); @@ -225,7 +227,7 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { Response jobSummaryResponse = thrift.getJobSummary(ROLE); assertEquals( - jobSummaryResponse(ownedImmedieteJobSummaryOnly), + jobSummaryResponse(ownedImmediateJobSummaryOnly), IResponse.build(jobSummaryResponse).newBuilder()); assertEquals(jobSummaryResponse(ImmutableSet.of()), thrift.getJobSummary(ROLE)); @@ -235,6 +237,33 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { } @Test + public void testGetJobSummaryWithoutNextRun() throws Exception { + // 31st of February, there is no such day. + String cronSchedule = "* * 31 2 *"; + + TaskConfig task = nonProductionTask() + .setJobName(JOB_KEY.getName()) + .setOwner(ROLE_IDENTITY) + .setEnvironment(JOB_KEY.getEnvironment()); + JobConfiguration job = makeJob() + .setCronSchedule(cronSchedule) + .setTaskConfig(task); + expect(cronPredictor.predictNextRun(CrontabEntry.parse(cronSchedule))) + .andReturn(Optional.absent()) + .anyTimes(); + storageUtil.expectTaskFetch(Query.roleScoped(ROLE)); + Set<JobConfiguration> jobOnly = ImmutableSet.of(job); + expect(storageUtil.jobStore.fetchJobs()) + .andReturn(IJobConfiguration.setFromBuilders(jobOnly)); + + control.replay(); + + JobSummaryResult result = thrift.getJobSummary(ROLE).getResult().getJobSummaryResult(); + assertEquals(1, result.getSummaries().size()); + assertFalse(result.getSummariesIterator().next().isSetNextCronRunMs()); + } + + @Test public void testGetPendingReason() throws Exception { Builder query = Query.unscoped().byJob(JOB_KEY); Builder filterQuery = Query.unscoped().byJob(JOB_KEY).byStatus(ScheduleStatus.PENDING);
