aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zma...@apache.org
Subject aurora git commit: Fixup `getJobSummary` for cron jobs with invalid next run dates.
Date Fri, 18 Dec 2015 17:28:55 GMT
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 <john.sirois@gmail.com>
Authored: Fri Dec 18 12:28:45 2015 -0500
Committer: Zameer Manji <zmanji@apache.org>
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);


Mime
View raw message