Add support for update to the Java test server #1762
Conversation
temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java
Outdated
Show resolved
Hide resolved
temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java
Show resolved
Hide resolved
| // If the workflow is finished we can't write more events | ||
| // to history so if the message was processed after the workflow | ||
| // was closed there is nothing we can do. | ||
| // The real server also has this same problem |
There was a problem hiding this comment.
What does the client-side caller of the update see in this situation?
Would love a to see tests for workflow-complete-before-update-accept (probably have to terminate while worker not running) and workflow-complete-before-update-complete (can just hang the update handler and finish the workflow).
There was a problem hiding this comment.
What does the client-side caller of the update see in this situation?
This does not impact what the caller side sees. I'll note this shouldn't be an issue once we have message commands.
There was a problem hiding this comment.
Would love a to see tests for workflow-complete-before-update-accept (probably have to terminate while worker not running)
I agree but we need the server to support UPDATE_WORKFLOW_EXECUTION_LIFECYCLE_STAGE_ADMITTED to write this test.
workflow-complete-before-update-complete (can just hang the update handler and finish the workflow).
I also agree, I was planning on covering this in the test server and the real server once they get support for UPDATE_WORKFLOW_EXECUTION_LIFECYCLE_STAGE_ACCEPTED
There was a problem hiding this comment.
I wonder if we should just error in workflow-completed situations then instead of silently succeeding (unless I'm misreading)
There was a problem hiding this comment.
I don't understand why we would error. I am emulating the real server and the real server does not error. Erroring would also not help us?
There was a problem hiding this comment.
I thought the real server just improperly handled this. Surely an update will error when workflow closed while processing when the real server is completed. I was just saying error here as "not-yet-properly implemented on real server so it intentionally isn't here either" until we see the shape that error takes. But we can just silently swallow now I guess too so long as we're willing to revisit with those two test scenarios before GA.
There was a problem hiding this comment.
There is an intentional implementation on the server as of yesterday. The guarantee is that a WFTCompletion will get an error back if there are any incomplete updates. If the workflow execution completed command comes in the same WFT as the update completion, that is allowed and the WFE can complete.
HOWEVER
Further discussion with the server team revealed that a different behavior is desired: we will (soon) prevent the WFE from completing only if there are updates that have not been seen by workflow code yet. In practice this means that updates in the admitted or requested state will prevent WFE completion. If we know that the workflow code has seen the update request (accepted or completed states) then the thinking is that the workflow had a chance to prevent it's own completion but chose not to do so and thus we let the completion go through and the update will never complete.
temporal-sdk/src/test/java/io/temporal/client/functional/UpdateTest.java
Show resolved
Hide resolved
| private static void bufferUpdate( | ||
| RequestContext ctx, WorkflowTaskData data, UpdateWorkflowExecution update, long notUsed) { | ||
| if (data.getUpdateRequest(update.getId()).isPresent()) { | ||
| throw Status.INTERNAL |
There was a problem hiding this comment.
Does this bubble back to the submitting client as an error?
| // If the workflow is finished we can't write more events | ||
| // to history so if the message was processed after the workflow | ||
| // was closed there is nothing we can do. | ||
| // The real server also has this same problem |
There was a problem hiding this comment.
There is an intentional implementation on the server as of yesterday. The guarantee is that a WFTCompletion will get an error back if there are any incomplete updates. If the workflow execution completed command comes in the same WFT as the update completion, that is allowed and the WFE can complete.
HOWEVER
Further discussion with the server team revealed that a different behavior is desired: we will (soon) prevent the WFE from completing only if there are updates that have not been seen by workflow code yet. In practice this means that updates in the admitted or requested state will prevent WFE completion. If we know that the workflow code has seen the update request (accepted or completed states) then the thinking is that the workflow had a chance to prevent it's own completion but chose not to do so and thus we let the completion go through and the update will never complete.
Add support for sync update to the test server.