Align Update API across test server and real server#2153
Align Update API across test server and real server#2153Quinn-With-Two-Ns merged 6 commits intotemporalio:masterfrom
Conversation
Quinn-With-Two-Ns
changed the title
Quinn-With-Two-Ns
force-pushed
the
issue-2142
branch
from
c69b277 to
c57ef50
Compare
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
Didn't look into details of test server behavior, but at a glance, I think this will make it usable in the tests it is currently skipped in now (e.g. Python).
dandavison
left a comment
dandavison
left a comment
There was a problem hiding this comment.
This looks great. Nice tests. I wonder whether @stephanos or @alexshtin could be persuaded to take a glance to see whether they notice anything and just generally to be aware of the parallel implementation.
| .setUpdateRef(updateHandle.getRef()) | ||
| .setStage(reachedStage); | ||
| if (reachedStage == UPDATE_WORKFLOW_EXECUTION_LIFECYCLE_STAGE_COMPLETED) { | ||
| response.setOutcome(updateHandle.getOutcomeNow()); |
There was a problem hiding this comment.
This is where you pointed out that the server doesn't actually always do this, right? Not sure if it's worth a comment in the code here.
There was a problem hiding this comment.
The issue with the Server is that reachedStage may only be ADMITTED, when the update was ADMITTED and COMPLETED in the same WFT. The test server also does not make any such guarantee.
| UpdateWorkflowExecution u = | ||
| workflowTaskStateMachine.getData().updateRequest.get(protocolInstanceId); | ||
| } |
There was a problem hiding this comment.
This isn't used, right? Should we be resolving the u.getOutcome() future in an onCommit() here? I guess I'm missing something because I doubt your tests would pass if we weren't doing that!
There was a problem hiding this comment.
Yes this isn't used i'll remove, u.getOutcome() is resolved in an onCommit it is just in the SM
| // completed | ||
| if (isTerminalState()) { | ||
| UpdateHandle updateHandle = getUpdate(request.getRequest().getMeta().getUpdateId()); | ||
| if (updateHandle.getOutcome().isDone()) { |
There was a problem hiding this comment.
I might well be forgetting something but why are we only doing this if the update is completed? I'm thinking a request with waitStage=Accepted that hits a completed workflow but where the update is Accepted should get the info for the Accepted update. See the diagram in temporalio/temporal#6085
There was a problem hiding this comment.
Hm i was just matching the serve behavior I observed in my tests. I agree according to your diagram I should change the logic here , but the current server does not follow that behavior. I think this test shows that https://github.com/temporalio/sdk-java/pull/2153/files#diff-d14da6956e0f5f91c5901dc5dd92e9207915ca200581176fa172fd85378aa742R634. Do you think the server has a bug here?
There was a problem hiding this comment.
It's true, the server implementation diverges from the diagram in that only completed Updates are returned; I didn't actually notice that when I worked on the server change and therefore didn't call that out.
There is an explicit test case in the Server PR for this case: https://github.com/temporalio/temporal/pull/6240/files#diff-30c0e64f181e0f2ee413c578ae22f3e159748ab746563bfdeed1a9f018340611R975
The reason it is this way is that the server will abort any pending updates when the workflow completes. And that intuitively makes sense to me: otherwise a client might poll for "completed" but receiving "accepted" for the update sent to a closed workflow will keep them polling indefinitely, right? So aborting it seems like an unavoidable step to me.
There was a problem hiding this comment.
otherwise a client might poll for "completed" but receiving "accepted"
In Dans diagram though I think that would still be an error. It is when the client polls for "accepted" should it receive an error.
| // If the workflow is in a terminal state, return the current state of the update if it | ||
| // completed | ||
| if (isTerminalState()) { | ||
| if (updateHandle.getOutcome().isDone()) { |
There was a problem hiding this comment.
Same question as for the other API
temporal-test-server/src/test/java/io/temporal/testserver/functional/WorkflowUpdateTest.java
Outdated
Show resolved
Hide resolved
temporal-test-server/src/test/java/io/temporal/testserver/functional/WorkflowUpdateTest.java
Outdated
Show resolved
Hide resolved
temporal-test-server/src/test/java/io/temporal/testserver/functional/WorkflowUpdateTest.java
Show resolved
Hide resolved
temporal-test-server/src/test/java/io/temporal/testserver/functional/WorkflowUpdateTest.java
Outdated
Show resolved
Hide resolved
|
It would be nice to test whether this makes tests pass that are currently skipping it, but that doesn't need to block merge of this PR if it's easier to merge first. |
Follow the Go SDK pattern (PR temporalio#2153) per reviewer feedback: 1. Use checkSdkFlag instead of tryUseSdkFlag so the flag is NOT auto-enabled for new workflows. Add TODO to switch to tryUseSdkFlag in the next release. 2. Remove CANCEL_AWAIT_TIMER_ON_CONDITION from initialFlags. 3. Tests explicitly toggle the flag to verify both old behavior (timer NOT cancelled) and new behavior (timer cancelled).
Follow the Go SDK pattern (PR temporalio#2153) per reviewer feedback: 1. Use checkSdkFlag instead of tryUseSdkFlag so the flag is NOT auto-enabled for new workflows. Add TODO to switch to tryUseSdkFlag in the next release. 2. Remove CANCEL_AWAIT_TIMER_ON_CONDITION from initialFlags. 3. Tests explicitly toggle the flag to verify both old behavior (timer NOT cancelled) and new behavior (timer cancelled).
4 participants
Align test server update APIs with real server update APIs. Key part is all the tests in temporal-test-server/src/test/java/io/temporal/testserver/functional/WorkflowUpdateTest.java to ensure the test server and real server behave the same. Some tests may be disabled as I found bugs in the real server while writing these tests.