Next natural trigger bug fix #27
Conversation
| patch.commitSha().isPresent() ? patch.commitSha() : o.commitSha()) | ||
| o -> { | ||
| WorkflowState.Builder builder = o.toBuilder() | ||
| .enabled(patch.enabled().orElse(originalWorkflowState.get().enabled().orElse(false))); |
There was a problem hiding this comment.
This line should use o instead of originalWorkflowState
| dockerImage.ifPresent(di -> builder.dockerImage(di)); | ||
| commitSha.ifPresent(cs -> builder.commitSha(cs)); | ||
| nextNaturalTrigger.ifPresent(nnt -> builder.nextNaturalTrigger(nnt)); | ||
| return builder.build(); |
There was a problem hiding this comment.
Why not just call the AutoValue_WorkflowState constructor?
There was a problem hiding this comment.
Because when I use AutoValue.Builder, the AutoValue_WorkflowState constructor becomes private. I didn't find a way of keeping it public.
There was a problem hiding this comment.
I guess all of these could be use method references rather than lambdas?
enabled.ifPresent(builder::enabled);
|
|
||
| injectEvent(Event.started(workflowInstance)); | ||
| injectEvent(Event.terminate(workflowInstance, 0)); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
Try using awaitWorkflowInstanceState instead of sleeping. I believe this line is waiting for the state to be in AWAITING_RETRY.
There was a problem hiding this comment.
This test verifies that after a wf has executed successfully with a given parameter, a change of its run specification wont re-trigger the WFI with the same parameter.
I want to check if the state is DONE, but using the awaitility didn't work because the state has been already processed (is not anymore in activeStates), so getState returns null. But I can just remove the sleeping assuming that the state will always reach DONE before starting a new statement?
There was a problem hiding this comment.
How about adding a way to await that a workflow instance completes? e.g. awaitWorkflowInstanceCompletion(wfi) that will wait until the state manager returns a null state'
| DATA_ENDPOINT_DAILY.secret()))); | ||
| timePasses(StyxScheduler.SCHEDULER_TICK_INTERVAL_SECONDS, SECONDS); | ||
|
|
||
| Thread.sleep(1000); |
There was a problem hiding this comment.
I didn't find Awaitility way to check that something has not changed.
|
Some comments, but looks good overall. The tests need some changes, plus a rebase. |
TinaRanic
force-pushed
the
nextNaturalTrigger-bugFix
branch
2 times, most recently
from
16eb6b8 to
6b8d18a
Compare
|
👍 |
| dockerImage.ifPresent(di -> builder.dockerImage(di)); | ||
| commitSha.ifPresent(cs -> builder.commitSha(cs)); | ||
| nextNaturalTrigger.ifPresent(nnt -> builder.nextNaturalTrigger(nnt)); | ||
| return builder.build(); |
There was a problem hiding this comment.
I guess all of these could be use method references rather than lambdas?
enabled.ifPresent(builder::enabled);| @@ -480,7 +485,7 @@ private TriggerListener trigger( | |||
| }; | |||
| } | |||
|
|
|||
| private static Consumer<Workflow> workflowChanged( | |||
| private Consumer<Workflow> workflowChanged( | |||
There was a problem hiding this comment.
Most other methods in this class seem to be static and get passed everything needed. Did you consider passing time into this method instead of making it an instance method?
| /** | ||
| * Fast forwards the time without any execution in-between. | ||
| */ | ||
| void timeJumps(int n, TimeUnit unit) { |
There was a problem hiding this comment.
Do you think it's somehow possible to have the distinction even clearer between timePasses and timeJumps without having to read the javadoc? jump compared to pass is in the right direction but I would definitely need to read the javadoc to know for sure.
Not a big deal at all, just thought it was unclear.
| getDockerImage(workflowId), | ||
| getCommitSha(workflowId)); | ||
| WorkflowState.Builder builder = WorkflowState.builder().enabled(enabled(workflowId)); | ||
| getDockerImage(workflowId).ifPresent(dockerImage -> builder.dockerImage(dockerImage)); |
| } | ||
|
|
||
| public static WorkflowState patchDockerImage(String dockerImage) { | ||
| return create(Optional.empty(), of(dockerImage), Optional.empty()); | ||
| return builder().dockerImage(dockerImage).build(); | ||
| } | ||
|
|
||
| public static WorkflowState patchEnabled(boolean enabled) { |
There was a problem hiding this comment.
These convenience methods don't make much sense after adding a builder.
There was a problem hiding this comment.
I agree, lets refactor that in another PR?
TinaRanic
force-pushed
the
nextNaturalTrigger-bugFix
branch
from
6b8d18a to
62d03e8
Compare
|
👍 |
Fixes #24