Repository navigation
Include memo in test server list workflow executions - #3182
Closed
shinseongsu wants to merge 1 commit into
Closed
shinseongsu wants to merge 1 commit into
shinseongsu wants to merge 1 commit into
Conversation
The test server built WorkflowExecutionInfo for ListOpenWorkflowExecutions and ListClosedWorkflowExecutions from history without the memo, so callers always got an empty memo even though DescribeWorkflowExecution and the real server return it. Rebuild the memo from the started event and every WorkflowPropertiesModified upsert, using the same merge rules as the mutable state. Fixes temporalio#2480
|
|
1 similar comment
|
|
Contributor
|
We're not accepting fixes for this sort of thing, since we are planning to begin deprecation for this server. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test server returned an empty memo for every entry from
ListOpenWorkflowExecutionsandListClosedWorkflowExecutions. For the same workflow,DescribeWorkflowExecutionreturned the memo, and so does the real server for both list calls:The list entries are built in
TestWorkflowStoreImpl.constructWorkflowExecutionInfofrom history alone, and that method never set the memo. Describe goes through the mutable state instead, which is why it was not affected.What was changed
TestWorkflowStoreImpl.constructWorkflowExecutionInfonow sets the memo, computed by a new privatememoFromHistory:WorkflowExecutionStartedeventWorkflowPropertiesModifiedevent, keeping the last value per keyStateUtils.mergeMemo, the same merge the mutable state uses, so anullupsert removes the key in both placesListWorkflowExecutionsMemoTest.CHANGELOG.mdentry under Fixed.No public API changes; everything is inside the internal test server.
Why?
Code that lists workflows and reads their memo works against
temporal server start-devbut sees an empty memo underTestWorkflowEnvironment, so it cannot be tested without a real server. Reported in #2480.Checklist
Closes Memo-s not present in listOpenWorkflowExecutions when running with temporal-testing #2480
How was this tested:
Correctness
ListWorkflowExecutionsMemoTeststarts a workflow with memo{startKey, removedKey}. The workflow upsertsupsertedKeyand removesremovedKeywith anullupsert. The test asserts that the open listing returns{startKey, upsertedKey}, then completes the workflow and asserts that the closed listing returns the same memo.expected:<{startKey=..., upsertedKey=...}> but was:<{}>Performance
The memo is now rebuilt from history on every list call, so I measured
ListOpenWorkflowExecutionsbefore and after the change. Setup: 200 open workflows, each started with one memo key and then upserting 0, 50 or 200 more keys. After 10 warm-up calls, 50 calls were timed withSystem.nanoTime()through the blocking stub against the in-process test server (Apple M4 Pro, macOS 15.6, OpenJDK 21.0.10, Gradle 8.10.2).With a few memo keys there is no noticeable difference. With many keys the call gets slower roughly in proportion to the response size, which is the memo that was previously missing. The memo work itself is linear in history length. My first version called
mergeMemoonce per upsert event, which copies the map every time, and took 72 ms (p50) for the 200-upsert case; collecting upserts per key and merging once brought that to 11.5 ms.Benchmark harness (not part of this PR)
Regression
:temporal-test-server:test(99 tests) and:temporal-sdk:test --tests "io.temporal.internal.testing.*"(35 tests) pass. In one of my full runsWorkflowIdConflictPolicyTest.conflictPolicyUseExistingfailed on thebufferedflag of a request id info. That test does not call the list APIs, so it never reaches the changed code, and rerunning it on its own passed 20/20 onmainand 20/20 with this change. It looks timing dependent: the flag depends on whether a workflow task is in flight when the second start request arrives.No.