Remove deprecated events #826
Conversation
ckiosidis
force-pushed
the
remove-depricated-events
branch
from
dd6249b
to
0ecdc24
Compare
| return state(RUNNING); | ||
| case PREPARE: | ||
| return state(RUNNING, data().builder() | ||
| .tries(data().tries() + 1) |
There was a problem hiding this comment.
@honnix do you know why we allow this transition from PREPARE directly to RUNNING? Is it in case we lose the events in between?
fyi, we added this change to keep track of the number of tries here because we found that this would be a path in the state machine that doesn't increment tries at any other transition. Not sure it's the right thing to do though.
There was a problem hiding this comment.
I don't know what the reason was. Missing tries increase on that transition branch was introduced by #33.
I doubt bypassing submitting and submitted states would actually work when executing a workflow instance because that would mean missing many things. During replay, this tries is not important I think.
My suggestion is we can remove support of this transition.
There was a problem hiding this comment.
BTW there are a few things in this file marked as backward compatibility, e.g. https://github.com/spotify/styx/pull/826/files#diff-05de7b680cea03b57e3a1df7bbbc1258R179 . I think we can also kill those.
There was a problem hiding this comment.
Cool, thanks for the input. I think we'll do that as part of a following PR just to keep this PR limited in scope.
Codecov Report
@@ Coverage Diff @@
## master #826 +/- ##
============================================
+ Coverage 91.62% 91.75% +0.12%
+ Complexity 1924 1921 -3
============================================
Files 177 177
Lines 7550 7510 -40
Branches 459 456 -3
============================================
- Hits 6918 6891 -27
+ Misses 521 510 -11
+ Partials 111 109 -2 |
| @@ -53,7 +53,7 @@ | |||
| import org.mockito.MockitoAnnotations; | |||
|
|
|||
| @RunWith(JUnitParamsRunner.class) | |||
| public class TimeoutHandlerTest { | |||
| public class TimeoutHandlerTest { | |||
There was a problem hiding this comment.
This seems to be unintended change.
| @@ -310,243 +296,234 @@ public void testRetryDelayFromQueued() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRetryFromRunError() { | |||
| public void testRetryAfterFromRunError() { | |||
There was a problem hiding this comment.
I think this test case meant to test a state after dequeue happens after retryAfter.
| assertThat(transitioner.get(WORKFLOW_INSTANCE).data().consecutiveFailures(), equalTo(1)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testManyRetriesFromRunError() { | ||
| public void testManyRetriesAfterFromRunError() { |
There was a problem hiding this comment.
I think this test case meant to test a state after dequeue happens after retryAfter.
| assertThat(transitioner.get(WORKFLOW_INSTANCE).data().consecutiveFailures(), equalTo(0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRetryFromTerm() { | ||
| public void testRetryAfterFromTerminated() { |
There was a problem hiding this comment.
I think this test case meant to test a state after dequeue happens after retryAfter.
| assertThat(transitioner.get(WORKFLOW_INSTANCE).data().consecutiveFailures(), equalTo(1)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testManyRetriesFromTerm() { | ||
| public void testManyRetriesAfterFromTerminated() { |
There was a problem hiding this comment.
I think this test case meant to test a state after dequeue happens after retryAfter.
styx-service-common/src/test/java/com/spotify/styx/storage/WFIExecutionBuilderTest.java
Show resolved
Hide resolved
styx-service-common/src/test/java/com/spotify/styx/storage/WFIExecutionBuilderTest.java
Show resolved
Hide resolved
styx-service-common/src/test/java/com/spotify/styx/storage/WFIExecutionBuilderTest.java
Show resolved
Hide resolved
styx-service-common/src/test/java/com/spotify/styx/storage/WFIExecutionBuilderTest.java
Show resolved
Hide resolved
styx-service-common/src/test/java/com/spotify/styx/storage/WFIExecutionBuilderTest.java
Show resolved
Hide resolved
Hey, I just made a Pull Request!
Description
Motivation and Context
Have you tested this? If so, how?
Checklist for PR author(s)
Checklist for PR reviewer(s)