Add support for CRON syntax for schedules #44

rouzwawi · 2017-01-24T16:28:55Z

todo:

Parse CRON syntax for Partitioning
Backwards compatible with previous Partitioning enum values
Add offset field supporting ISO 8601 Durations for offsetting trigger time
Align Workflow Instance parameter generation for general CRON Partitioning
Persist offset trigger time (for API)
Workflow Instance parameter parsing in Scheduler trigger API
Documentation
Lower interval limit (pushed for later PR)

fixes #37

luster · 2017-03-20T19:17:00Z

is there a release timeline for this feature? @rouzwawi

danielnorberg · 2017-03-21T15:35:02Z

Does this mean that a workflow can be scheduled to run once a minute?

rouzwawi · 2017-03-21T17:35:47Z

@danielnorberg yes. I was thinking of adding a lower limit on the schedule interval. Running a new pod every minute can definitely cause issues.

rouzwawi · 2017-03-21T17:36:46Z

@luster We're looking into merging this soon. Rough guess would around a week from now.

danielnorberg · 2017-03-24T12:54:32Z

Would it be relevant to consider the smearing issue in this PR as well?

#110

rouzwawi · 2017-03-24T22:03:04Z

@danielnorberg I would prefer to do it in a separate PR, as I don't think it should be implemented on the triggering layer. See my comment in the issue.

Uses default offsets for well known cron specifications such as HOURLY, DAILY, etc. This will preserve the current behaviour of running one period later than the time in the instance parameter.

honnix · 2017-03-28T07:10:52Z

styx-common/src/main/java/com/spotify/styx/storage/AggregateStorage.java

  }

  @Override
-  public Map<Workflow, Optional<Instant>> workflowsWithNextNaturalTrigger()
-      throws IOException {
+  public void updateNextNaturalTriggerOld(WorkflowId workflowId, Instant instant) throws IOException {


It seems this method is not used anywhere. Still keeping it?

It's used from a test that simulates the storage condition before migrating to this. Will add a VisibleForTesting annotation.

honnix · 2017-03-28T07:13:28Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

-      throws IOException {
-    Map<Workflow, Optional<Instant>> map = Maps.newHashMap();
+  public Map<Workflow, TriggerInstantSpec> workflowsWithNextNaturalTrigger() throws IOException {
+    Map<Workflow, TriggerInstantSpec> map = Maps.newHashMap();


honnix · 2017-03-28T07:13:39Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+
+        map.put(workflow, TriggerInstantSpec.create(instant, triggerInstant));
+      }
+


honnix · 2017-03-28T07:14:26Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

@@ -265,16 +286,31 @@ public void updateNextNaturalTrigger(WorkflowId workflowId, Instant nextNaturalT
      final Entity entity = result.next();
      Workflow workflow;


honnix · 2017-03-28T07:16:04Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+        Instant instant = datetimeToInstant(entity.getDateTime(PROPERTY_NEXT_NATURAL_TRIGGER));
+        Instant triggerInstant;
+
+        // todo: this check is only needed during a transition period


Would you like to perform a data migration by adding a new property now? Or let it migrate itself when updating natural trigger?

honnix · 2017-03-28T07:16:41Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+
+      if (entity.contains(PROPERTY_NEXT_NATURAL_TRIGGER)) {
+        Instant instant = datetimeToInstant(entity.getDateTime(PROPERTY_NEXT_NATURAL_TRIGGER));
+        Instant triggerInstant;


honnix · 2017-03-28T07:22:36Z

styx-common/src/main/java/com/spotify/styx/util/ParameterUtil.java


  private ParameterUtil() {
  }

  private static final int MIN_YEAR_WIDTH = 4;
  private static final int MAX_YEAR_WIDTH = 10;
+
+  private static final DateTimeFormatter DATE_HOUR_FORMAT = new DateTimeFormatterBuilder()


This stack alike builder is not quite readable. Hopefully we will never need to change it.

danielnorberg · 2017-03-28T07:23:42Z

styx-api-service/src/test/java/com/spotify/styx/api/deprecated/BackfillResourceTest.java

@@ -153,18 +153,18 @@ public void setUp() throws Exception {
    storage.storeWorkflow(Workflow.create(
        BACKFILL_1.workflowId().componentId(), URI.create("http://example.com"),
        WorkflowConfiguration.create(BACKFILL_1.workflowId().id(), Schedule.HOURS,
-                                     Optional.empty(), Optional.empty(), Optional.empty(),
-                                     Optional.empty(), Collections.emptyList())));
+                                     empty(), empty(), empty(), empty(),


There's an argument for using builders somewhere in these lines ;)

Yes :) I wonder if there's a good library for that ;)

danielnorberg · 2017-03-28T07:25:08Z

styx-common/src/main/java/com/spotify/styx/model/Schedule.java

+  }
+
+  public enum WellKnown {
+    HOURLY, DAILY, WEEKLY, MONTHLY, YEARLY, UNKNOWN


WellKnown.UNKNOWN 😄

honnix · 2017-03-28T07:29:16Z

styx-common/src/main/java/com/spotify/styx/util/ParameterUtil.java

+    final Schedule.WellKnown wellKnown = schedule.wellKnown();
+
+    Matcher matcher;
+    ZonedDateTime parsed;


honnix · 2017-03-28T08:13:21Z

styx-scheduler-service/src/main/java/com/spotify/styx/workflow/WorkflowInitializer.java

+      previous = storage.workflow(workflow.id());
+      storage.storeWorkflow(workflow);
+    } catch (IOException e) {
+      throw Throwables.propagate(e);


@danielnorberg found this https://github.com/google/guava/wiki/Why-we-deprecated-Throwables.propagate, so I would suggest for new code we don't use propagate.

Good read. I had started questioning the usefulness of Throwables.propagate.

honnix · 2017-03-28T08:25:48Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+  }
+
+  @Deprecated
+  public void updateNextNaturalTrigger(WorkflowId workflowId, Instant instant) throws IOException {


Same here. Do you still need this method?

honnix · 2017-03-28T08:37:38Z

styx-scheduler-service/src/main/java/com/spotify/styx/StyxScheduler.java

-
-    final Consumer<Workflow> workflowChangeListener = workflowChanged(workflowCache, storage,
-                                                                      stats, stateManager, time);
+    final WorkflowInitializer workflowInitializer = new WorkflowInitializer(storage, time);


inline this maybe as it's only used by workflowChangeListener? or move it closer to where it is used.

It would just push down more of the dependencies (storage, time) into the workflowChanged handler which does not use them at the moment. So it's a net +1 dependency. I'll keep it here.

I didn't mean that change. Just

final Consumer<Workflow> workflowChangeListener = workflowChanged(workflowCache, new WorkflowInitializer(storage, time), stats, stateManager);

But it's OK.

honnix · 2017-03-28T08:48:26Z

A few minor comments.

danielnorberg · 2017-03-28T09:00:29Z

styx-common/src/main/java/com/spotify/styx/util/TriggerInstantSpec.java

+  public abstract Instant instant();
+
+  /**
+   * The actual instant at which the Workflow will be instantiated, with respect to the


s/instantiated/triggered ?

danielnorberg · 2017-03-28T12:49:47Z

LGTM overall 👍

rouzwawi · 2017-03-28T15:04:52Z

@honnix @danielnorberg Fixed review comments

rouzwawi force-pushed the cron branch 10 times, most recently from e361f3b to 3d241e4 Compare January 27, 2017 13:37

rouzwawi force-pushed the cron branch 3 times, most recently from bcb256d to 5c6d666 Compare February 10, 2017 18:38

rouzwawi force-pushed the cron branch 2 times, most recently from 305231d to 067cea2 Compare February 21, 2017 22:16

rouzwawi force-pushed the cron branch 6 times, most recently from 2a05a2d to ba81f4a Compare March 22, 2017 18:59

rouzwawi force-pushed the cron branch 2 times, most recently from c8bd435 to e8917d3 Compare March 24, 2017 21:54

rouzwawi force-pushed the cron branch from 1a4f8ec to 23a1fe5 Compare March 24, 2017 22:38

rouzwawi added 8 commits March 27, 2017 10:43

Apply offset on next trigger instant

90a413d

Uses default offsets for well known cron specifications such as HOURLY, DAILY, etc. This will preserve the current behaviour of running one period later than the time in the instance parameter.

Test custom cron schedule

2bc42eb

Add TriggerInstantSpec and persist next trigger with offset

d981d4e

Clean up last of ParameterUtil

b946f52

Update README with cron specs

08debe9

Align parsed workflow instance parameter according to schedule

f7e1fc3

Change partition to schedule in some docs

706523e

Make sure to trigger the right partition for migration

e3e8928

rouzwawi force-pushed the cron branch from fd59e60 to 706a9e3 Compare March 27, 2017 14:44

honnix reviewed Mar 28, 2017

View reviewed changes

danielnorberg reviewed Mar 28, 2017

View reviewed changes

honnix reviewed Mar 28, 2017

View reviewed changes

danielnorberg reviewed Mar 28, 2017

View reviewed changes

Review fixes

829da8b

rouzwawi force-pushed the cron branch from 66d794d to 829da8b Compare March 28, 2017 15:02

rouzwawi merged commit cad991b into master Mar 28, 2017

rouzwawi deleted the cron branch March 28, 2017 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CRON syntax for schedules #44

Add support for CRON syntax for schedules #44

rouzwawi commented Jan 24, 2017 •

edited

Loading

luster commented Mar 20, 2017

danielnorberg commented Mar 21, 2017 •

edited

Loading

rouzwawi commented Mar 21, 2017

rouzwawi commented Mar 21, 2017

danielnorberg commented Mar 24, 2017

rouzwawi commented Mar 24, 2017

honnix Mar 28, 2017

rouzwawi Mar 28, 2017

honnix Mar 28, 2017

honnix Mar 28, 2017

honnix Mar 28, 2017

honnix Mar 28, 2017 •

edited

Loading

honnix Mar 28, 2017

honnix Mar 28, 2017

danielnorberg Mar 28, 2017

danielnorberg Mar 28, 2017

rouzwawi Mar 28, 2017

danielnorberg Mar 28, 2017 •

edited

Loading

honnix Mar 28, 2017

honnix Mar 28, 2017

rouzwawi Mar 28, 2017

honnix Mar 28, 2017

honnix Mar 28, 2017

rouzwawi Mar 28, 2017

honnix Mar 28, 2017

honnix commented Mar 28, 2017

danielnorberg Mar 28, 2017

danielnorberg commented Mar 28, 2017

rouzwawi commented Mar 28, 2017


		map.put(workflow, TriggerInstantSpec.create(instant, triggerInstant));
		}

		@@ -265,16 +286,31 @@ public void updateNextNaturalTrigger(WorkflowId workflowId, Instant nextNaturalT
		final Entity entity = result.next();
		Workflow workflow;

Add support for CRON syntax for schedules #44

Add support for CRON syntax for schedules #44

Conversation

rouzwawi commented Jan 24, 2017 • edited Loading

luster commented Mar 20, 2017

danielnorberg commented Mar 21, 2017 • edited Loading

rouzwawi commented Mar 21, 2017

rouzwawi commented Mar 21, 2017

danielnorberg commented Mar 24, 2017

rouzwawi commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnorberg Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix commented Mar 28, 2017

Choose a reason for hiding this comment

danielnorberg commented Mar 28, 2017

rouzwawi commented Mar 28, 2017

rouzwawi commented Jan 24, 2017 •

edited

Loading

danielnorberg commented Mar 21, 2017 •

edited

Loading

honnix Mar 28, 2017 •

edited

Loading

danielnorberg Mar 28, 2017 •

edited

Loading