Triggers transition into QUEUED state #26
Conversation
fabriziodemaria
force-pushed
the
queue-triggers
branch
2 times, most recently
from
882e425 to
fd1872e
Compare
| public RunState submit(WorkflowInstance workflowInstance, ExecutionDescription executionDescription) { | ||
| switch (state()) { | ||
| case QUEUED: // for backwards compatibility |
There was a problem hiding this comment.
This probably also have to be added for the legacy created event.
There was a problem hiding this comment.
Since we added the QUEUED state after deprecating created, we should not have in storage a created transition that was executed on top of a state machine that includes the QUEUED state. Do we really need to allow for that transition in RunState?
There was a problem hiding this comment.
Never mind! Modifying triggerExecution behaviour indeed means we need to allow for the created transition from QUEUED.
| @@ -101,6 +101,7 @@ public void testRunErrorOnCreating() throws Exception { | |||
| public void testSetExecutionId() throws Exception { | |||
| transitioner.initialize(RunState.fresh(WORKFLOW_INSTANCE, this::record)); | |||
| transitioner.receive(eventFactory.triggerExecution("trig")); | |||
| transitioner.receive(eventFactory.dequeue()); | |||
There was a problem hiding this comment.
I think these tests should not be modified to add a dequeue event but instead the state machine has to be backwards compatible with triggerExecution -> created sequences. See comment above.
|
|
||
| @Test | ||
| public void shouldExecuteNewTriggers() throws Exception { | ||
| StateData stateData = StateData.builder().tries(0).build(); | ||
| setUp(20, RunState.create(INSTANCE, State.QUEUED, stateData, time)); | ||
|
|
||
| now = now.plus(15, ChronoUnit.SECONDS); |
There was a problem hiding this comment.
Maybe this line should be removed so that we ensure that it dequeues the state immediately?
| givenStoredEvent(Event.retryAfter(workflowInstance, 30000), 13L); | ||
| givenActiveStateAtSequenceCount(workflowInstance, 13L); | ||
| givenStoredEvent(Event.triggerExecution(workflowInstance, "trig1"), 0L); | ||
| givenStoredEvent(Event.dequeue(workflowInstance), 1L); |
There was a problem hiding this comment.
Same comment here as in the RunState test, this test should pass without modification.
| @@ -64,23 +64,29 @@ public void shouldCatchUpWithNaturalTriggers() throws Exception { | |||
| givenNextNaturalTrigger(HOURLY_WORKFLOW, "2016-03-14T13:00:00Z"); | |||
|
|
|||
| styxStarts(); | |||
| timePasses(1, SECONDS); | |||
| tickTriggerManager(); | |||
| Thread.sleep(100); | |||
There was a problem hiding this comment.
Can this be implemented without a sleep? Perhaps use awaitility to wait for the condition you had in mind.
This also introduces the new dequeue event.
fabriziodemaria
force-pushed
the
queue-triggers
branch
from
fd1872e to
75c09c3
Compare
|
I think this looks good. It was a bit unclear why some of the tests were modified to add the Seems all of the ones I tried was the latter, so it's ok. For future changes, make it clear if a test was changed to align or to fix failures. |
|
Thanks for pointing that out. I will make my intentions clear with comments the next time tests are changed this way. |
|
👍 |
No description provided.