Introduce Queued State #10

rouzwawi · 2016-11-30T17:20:48Z

This is mainly a refactor of existing behaviour into a setup that makes it easier to implement further scheduling behaviour.

The change also renames the AWAITING_RETRY state to a more generic QUEUED state.

> STYX_CLI_HOST=localhost:8080 styx ls -c example-component
  WORKFLOW INSTANCE    STATE            LAST EXECUTION ID                               PREVIOUS EXECUTION INFO

example-component bihourly-job
  2016-12-01           QUEUED           styx-run-bc83472a-f4e8-4bab-a86a-070bf6649054   Exit code: 20
  2016-12-02           QUEUED           styx-run-212ef3f0-8ae3-4f56-b969-75319ef93cda   Exit code: 20
  2016-12-03           QUEUED           styx-run-6a03f8de-8b69-42e0-8615-d00371ae5afe   Exit code: 20

bergman · 2016-12-06T15:27:03Z

styx-scheduler-service/src/main/java/com/spotify/styx/state/StateManager.java

@@ -58,6 +59,11 @@
  void receive(Event event) throws IsClosed;

  /**
+   * Get a map of all active {@link WorkflowInstance} states.
+   */
+  ImmutableMap<WorkflowInstance, RunState> activeStates();


Why ImmutableMap and not just Map? Would anyone think they can mutate the return value of this and somehow that would be persisted?

Will replace with just Map

bergman · 2016-12-06T15:31:08Z

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

+    return isAtOrAfter(now, deadline);
+  }
+
+  private boolean isAtOrAfter(Instant now, Instant deadline) {


I find this function confusing, I'm not sure which value is at or after compared to the other and isn't this the same as !foo.isAfter(bar)?

bergman · 2016-12-06T15:36:06Z

LGTM so far but I guess we need to redirect the old events (triggerExec, submit) to this QUEUED state?

fabriziodemaria · 2016-12-06T14:29:15Z

styx-common/src/main/java/com/spotify/styx/state/RunState.java

@@ -202,7 +157,12 @@ public RunState triggerExecution(WorkflowInstance workflowInstance, String trigg
    public RunState created(WorkflowInstance workflowInstance, String executionId, String dockerImage) {
      switch (state()) {
        case PREPARE:


For clarity we should add // for backwards compatibility comment.

fabriziodemaria · 2016-12-06T14:47:00Z

styx-common/src/main/java/com/spotify/styx/state/RunState.java


        default:
          throw illegalTransition("runError");
      }
    }

    @Override
+    public RunState enqueue(WorkflowInstance workflowInstance) {


Can we use retryAfter instead of introducing a new enqueue event? We would reduce the need for "supporting" deprecated events/transitions.

We could also modify retryAfter to drop the delayMillis, which is not needed anymore with the new Scheduler. The only drawback I see with this approach is that the name retryAfter would be a bit confusing without an explicit delay argument in the constructor.

Also, without an explicit delay in the event (either enqueue or retryAfter), we would lose the possibility to return the expected delay in the CLI events command. Perhaps we could keep retryAfter as is for now, and remove the delayMillis when introducing the info event.

fabriziodemaria · 2016-12-06T14:47:45Z

styx-common/src/main/java/com/spotify/styx/state/RunState.java

@@ -286,7 +290,11 @@ public RunState retryAfter(WorkflowInstance workflowInstance, long delayMillis)
      switch (state()) {
        case TERMINATED:
        case FAILED:


If we introduce enqueue, the two lines above should include // for backwards compatibility

I removed the enqueue and dequeue events from this PR, so it's mainly focused on just introducing the QUEUED state.

fabriziodemaria · 2016-12-06T15:01:45Z

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

@@ -212,7 +216,7 @@ public StyxScheduler build() {
          scheduleSources,
          statsFactory,
          executorFactory,
-          publisherFactory);
+          publisherFactory, retryUtil);


Should have its own new line :)

fabriziodemaria · 2016-12-06T15:16:50Z

styx-scheduler-service/src/main/java/com/spotify/styx/state/handlers/TerminationHandler.java

-    final Duration delay = baseDelay.multipliedBy(multiplier);
-
-    final String instanceKey = state.workflowInstance().toKey();
-    LOG.info("{} scheduling retry #{} in {}", instanceKey, state.data().tries(), delay);


Is it intended to leave out this logging?

fabriziodemaria · 2016-12-06T15:40:25Z

styx-common/src/main/java/com/spotify/styx/state/RunState.java

@@ -298,7 +306,7 @@ public RunState retry(WorkflowInstance workflowInstance) {
      switch (state()) {
        case TERMINATED: // for backwards compatibility
        case FAILED:     // for backwards compatibility
-        case AWAITING_RETRY:
+        case QUEUED:


// for backwards compatibility

rouzwawi · 2016-12-06T16:21:12Z

@bergman I've deferred redirecting triggerExecution and submit into QUEUED to se later PR.

bergman · 2016-12-07T09:59:22Z

👍

rouzwawi force-pushed the queued branch 8 times, most recently from babac40 to fdebd1c Compare December 4, 2016 16:45

bergman reviewed Dec 6, 2016

View reviewed changes

fabriziodemaria reviewed Dec 6, 2016

View reviewed changes

rouzwawi force-pushed the queued branch 3 times, most recently from 5336c0d to e85708e Compare December 6, 2016 17:01

rouzwawi added 7 commits December 6, 2016 18:46

Use volatile fields for latches

78d0bb8

Refactor state data into value type

d54e41b

Add QUEUED state

b01e54c

Refactor retry calculation into utility

b2b719d

Start TriggerManager tick the same way as other background tasks

3063e71

Extract retries and timeouts to Scheduler class

916438e

Review fixes

43c17ce

rouzwawi force-pushed the queued branch from 350bc31 to 78d0bb8 Compare December 6, 2016 17:46

rouzwawi merged commit 77cc992 into master Dec 7, 2016

rouzwawi deleted the queued branch December 7, 2016 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Queued State #10

Introduce Queued State #10

rouzwawi commented Nov 30, 2016 •

edited

Loading

bergman Dec 6, 2016

rouzwawi Dec 6, 2016

bergman Dec 6, 2016

bergman commented Dec 6, 2016

fabriziodemaria Dec 6, 2016

fabriziodemaria Dec 6, 2016 •

edited

Loading

fabriziodemaria Dec 6, 2016

rouzwawi Dec 6, 2016

fabriziodemaria Dec 6, 2016

fabriziodemaria Dec 6, 2016

rouzwawi Dec 6, 2016

fabriziodemaria Dec 6, 2016

rouzwawi commented Dec 6, 2016

bergman commented Dec 7, 2016

Introduce Queued State #10

Introduce Queued State #10

Conversation

rouzwawi commented Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergman commented Dec 6, 2016

Choose a reason for hiding this comment

fabriziodemaria Dec 6, 2016 • 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

rouzwawi commented Dec 6, 2016

bergman commented Dec 7, 2016

rouzwawi commented Nov 30, 2016 •

edited

Loading

fabriziodemaria Dec 6, 2016 •

edited

Loading