functional tests testing queries + nexus tasks with versioning-3 #7015

Shivs11 · 2024-12-19T19:08:59Z

What changed?

various functional tests (sticky, no-sticky) testing Query + Nexus tasks routing with versioning-3
refactored taskPoller to now consider query and nexus tasks

Why?

more testing

How did you test it?

no feature related line changes so N/A

Potential risks

none: shall reduce risks if not for anything

Documentation

Is hotfix candidate?

No

service/history/api/get_workflow_util.go

common/testing/taskpoller/taskpoller.go

ShahabT · 2024-12-24T19:31:59Z

tests/versioning_3_test.go

+	s.verifyWorkflowVersioning(tv, vbUnpinned, tv.Deployment(), nil, nil)
+	if sticky {
+		s.verifyWorkflowStickyQueue(we, tv.StickyTaskQueue())
+	}


Let's add a query redirect step to this test. After this line, we should change the current deployment, and query the wf and make sure the new deployment's poller receives the query.

ShahabT · 2024-12-24T19:37:41Z

common/testing/taskpoller/taskpoller.go

@@ -125,9 +146,123 @@ func (p *TaskPoller) PollAndHandleWorkflowTask(
 // If no task is available, it returns NoTaskAvailable.
 func (p *workflowTaskPoller) HandleTask(


Most of the time we do not work with queries, it seems a little to much to impose the nested TaskCompletedResponse on all usages. Why not keep all these methods untouched and add a HandleQueryTask instead:

func (p *workflowTaskPoller) HandleQueryTask(tv *testvars.TestVars, handler func(task *workflowservice.PollWorkflowTaskQueueResponse) (*workflowservice.RespondQueryTaskCompletedResponse, error)

stephanos · 2024-12-24T19:53:22Z

common/testing/taskpoller/taskpoller.go

+	TaskCompletedRequest struct {
+		WorkflowTaskCompletedRequest *workflowservice.RespondWorkflowTaskCompletedRequest
+		QueryTaskCompletedRequest    *workflowservice.RespondQueryTaskCompletedRequest
+	}
+	TaskCompletedResponse struct {
+		WorkflowTaskCompletedResponse *workflowservice.RespondWorkflowTaskCompletedResponse
+		QueryTaskCompletedResponse    *workflowservice.RespondQueryTaskCompletedResponse
+	}


Hm, I'm not sure this is the right direction. I understand that the current API doesn't work for these Queries, but 95% of the time, the task poller will probably only deal with WFTs. Right now, without thinking longer about it, I see three other directions:

Create a dedicated, exported function just for queries.

Change the return type to any and require type assertions.

Hide the fact that there's a "legacy" query (AFAIU) and use the "new" query API; but inside the taskpoller do the right thing auto-magically

(1) would be the easiest and clearest; let me know if that's a feasible option. I haven't worked with this Query API before.

tagging you @ShahabT here as well since you had the exact concern - The main point seems to be to remove the need for TaskCompletedResponse/Request structures.

In my earlier stab at this, I did not have any of these structures and had made one exported function inside (*workflowTaskPoller).handleTask and had changed the return type to any (all the places that required it). Not, that in this attempt I also intended on using Versioning3Suite.pollWftAndHandle since it served as a nice helper function for all my needs. My train of thought was:

That "having type assertions and changing the return type to nil" or having these two structures were effectively conveying the same purpose: The response can be one of two types - Query or Workflow

Having understood this, if I were reading the codebase for the first time, I realized I would appreciate knowing that the handler deals with only Queries and Workflow related tasks (which is made clear by the structure in place since it has only those two fields vs any would make it a bit confusing)

Hence, I took the more tedious approach and went ahead with this option - I can certainly revert back if you both feel strongly of not keeping this the way it is but would be curious to hear your thoughts

Your reasoning is totally reasonable; but I strongly believe it's not the right path because it's too verbose for the majority of use cases. That was already a concern before, and is exacerbated here.

Why not keep all these methods untouched and add a HandleQueryTask instead:

To pick up Shahab's suggestion; is this feasible in your opinion? If so, it would be my preferred approach.

Will be attempting to have type assertions in place along with a separate HandleQueryTask method ⏳

common/testing/taskpoller/taskpoller.go

tests/versioning_3_test.go

Shivs11 · 2025-01-06T21:58:27Z

tests/versioning_3_test.go

@@ -804,6 +1051,78 @@ func (s *Versioning3Suite) pollWftAndHandle(
 	return nil, nil
 }

+func (s *Versioning3Suite) pollWftAndHandleQueries(


this was the giant guy I was trying to avoid all along which made me go down the rabbit hole of making those structures which in-turn stirred up the whole thread :(

cc - @stephanos @ShahabT

tests/versioning_3_test.go

common/testing/taskpoller/taskpoller.go

tests/versioning_3_test.go

common/testing/taskpoller/taskpoller.go

stephanos · 2025-01-07T00:07:19Z

common/testing/taskpoller/taskpoller.go

@@ -262,6 +389,51 @@ func (p *workflowTaskPoller) pollTask(
 	return resp, err
 }

+func (p *workflowTaskPoller) HandleQueries(


I think it would be best to signal that this is only for a legacy query. Like HandleLegacyQuery. (same for the other methods below)

I think this is a good point - having said that, do we need to add in more tests for the updated/latest queries? I ended up choosing this one (noticed the new queries only later) since I figured we only wanted to check if the functionality with routing was working or not with versioning-3 changes

cc @ShahabT

Is there anything "legacy" about these query responses? (I see that they are called legacy in this proto)
But I don't think it's a good choice of word. Maybe something like standalone query fits better because these queries and RespondQueryTaskCompleted request/responses are normally used and they are not deprecated in any sense as one would think by seeing them described as "legacy". Unless I'm mistaken, if there is other work to do in the wf, the query will be stuffed into the anyways-needed wft. Otherwise it'll be sent as standalone query (still a wft but with less info), an in that case SDK will send a RespondQueryTaskCompletedRequest instead of a RespondWorkflowTaskCompletedRequest.

I agree, it felt a bit odd for me too to be calling something legacy but actively use it in some of my work here - maybe this is a better question for the SDK folks and I can take this up with them;

I shall keep the function names with Legacy for now to respect which proto fields we are respecting but if we do see some changes in the proto side, I will alter the tests.

I'm okay with a different name.

Having said that, it seems like this name stuck now as it's in the docs and used in our internal communication, too (searched Slack). I fear a different name - even if better/more accurate - only creates confusion. But I'm not blocking.

common/testing/taskpoller/taskpoller.go

Shivs11

thank you for the feedback!

tests/versioning_3_test.go

Shivs11 · 2025-01-07T16:45:25Z

common/testing/taskpoller/taskpoller.go

@@ -262,6 +389,51 @@ func (p *workflowTaskPoller) pollTask(
 	return resp, err
 }

+func (p *workflowTaskPoller) HandleQueries(


I think this is a good point - having said that, do we need to add in more tests for the updated/latest queries? I ended up choosing this one (noticed the new queries only later) since I figured we only wanted to check if the functionality with routing was working or not with versioning-3 changes

cc @ShahabT

common/testing/taskpoller/taskpoller.go

tests/versioning_3_test.go

stephanos

👍 LGTM from taskpoller perspective. I'll defer to Shahab for the rest.

Shivs11 · 2025-01-08T15:32:55Z

tests/versioning_3_test.go

+	go s.idlePollWorkflow(tv, true, ver3MinPollTime, "old deployment should not receive query")
+	// Since the current deployment has changed, task will move to the normal queue (thus, sticky=false)
+	s.pollAndQueryWorkflow(tvB, false)
+


I was able to catch some flakes happening because the previous poller, with deployment set to tvB, was still up and running when I tried to poll and process a new query. This is fixed by ensuring the first poller dies before we continue the test.

ShahabT

LGTM

Added functional tests testing queries for versioning-3

093ab55

Shivs11 requested a review from a team as a code owner December 19, 2024 19:09

Shivs11 requested review from ShahabT, carlydf and dnr and removed request for carlydf December 19, 2024 19:09

Shivs11 commented Dec 19, 2024

View reviewed changes

service/history/api/get_workflow_util.go Show resolved Hide resolved

Shivs11 commented Dec 19, 2024

View reviewed changes

common/testing/taskpoller/taskpoller.go Show resolved Hide resolved

tests to verify routing of nexus tasks using versioning-3;

876b7f6

Shivs11 changed the title ~~Added functional tests testing queries for versioning-3~~ functional tests testing queries + nexus tasks with versioning-3 Dec 20, 2024

Shivs11 added 3 commits December 20, 2024 10:03

goimports

deec714

added waiting until poller completion

1024efa

lint fixes

02c9f46

ShahabT reviewed Dec 24, 2024

View reviewed changes

stephanos reviewed Dec 24, 2024

View reviewed changes

query redirects to latest deployment

ff38738

Shivs11 commented Dec 30, 2024

View reviewed changes

tests/versioning_3_test.go Show resolved Hide resolved

Shivs11 added 6 commits January 6, 2025 16:12

Addressed comments by making separate helpers

37b1672

removing non-required comments

61742a4

Merge branch 'main' into shivam/query-versioning3-func-tests

24a50cd

lint errors

0750ef0

better code

9e37261

removed stale comments

b854081

Shivs11 commented Jan 6, 2025

View reviewed changes

tests/versioning_3_test.go Outdated Show resolved Hide resolved

Shivs11 requested review from ShahabT and stephanos January 6, 2025 22:01

ShahabT reviewed Jan 6, 2025

View reviewed changes

tests/versioning_3_test.go Outdated Show resolved Hide resolved

tests/versioning_3_test.go Outdated Show resolved Hide resolved

tests/versioning_3_test.go Outdated Show resolved Hide resolved

stephanos reviewed Jan 7, 2025

View reviewed changes

Shivs11 added 2 commits January 7, 2025 12:51

addressed all comments

1725f90

restoring old code

6b96b98

Shivs11 commented Jan 7, 2025

View reviewed changes

stephanos reviewed Jan 7, 2025

View reviewed changes

common/testing/taskpoller/taskpoller.go Outdated Show resolved Hide resolved

tests/versioning_3_test.go Outdated Show resolved Hide resolved

Shivs11 added 2 commits January 7, 2025 14:08

Fixed errors

6bd2fdc

Setting query result on a nil error

966a075

stephanos approved these changes Jan 7, 2025

View reviewed changes

Shivs11 added 3 commits January 7, 2025 22:32

updating tests

dba609d

quieting the lint

9bf96e3

removing time.Sleep from tests

114a87d

Shivs11 commented Jan 8, 2025

View reviewed changes

ShahabT approved these changes Jan 8, 2025

View reviewed changes

Shivs11 merged commit 7d95a34 into main Jan 8, 2025
49 checks passed

Shivs11 deleted the shivam/query-versioning3-func-tests branch January 8, 2025 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

functional tests testing queries + nexus tasks with versioning-3 #7015

functional tests testing queries + nexus tasks with versioning-3 #7015

Shivs11 commented Dec 19, 2024 •

edited

Loading

ShahabT Dec 24, 2024

ShahabT Dec 24, 2024

stephanos Dec 24, 2024

stephanos Dec 24, 2024

Shivs11 Dec 25, 2024

stephanos Jan 6, 2025

Shivs11 Jan 6, 2025

Shivs11 Jan 6, 2025

stephanos Jan 7, 2025

Shivs11 Jan 7, 2025

ShahabT Jan 8, 2025

Shivs11 Jan 8, 2025

stephanos Jan 8, 2025 •

edited

Loading

Shivs11 left a comment

Shivs11 Jan 7, 2025

stephanos left a comment •

edited

Loading

Shivs11 Jan 8, 2025

ShahabT left a comment

		@@ -125,9 +146,123 @@ func (p *TaskPoller) PollAndHandleWorkflowTask(
		// If no task is available, it returns NoTaskAvailable.
		func (p *workflowTaskPoller) HandleTask(

functional tests testing queries + nexus tasks with versioning-3 #7015

functional tests testing queries + nexus tasks with versioning-3 #7015

Conversation

Shivs11 commented Dec 19, 2024 • edited Loading

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanos Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Shivs11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShahabT left a comment

Choose a reason for hiding this comment

Shivs11 commented Dec 19, 2024 •

edited

Loading

stephanos Jan 8, 2025 •

edited

Loading

stephanos left a comment •

edited

Loading