ListWorkerDeployments : Versioning-3.1 #7173

Shivs11 · 2025-01-28T13:47:54Z

What changed?

ListWorkerDeployments API
Also removed presence of locks when adding a version to a worker-deployment workflow. Causes deadlocks and we don't need them.

Why?

Versioning-3.1

How did you test it?

Added functional tests (happy-path)
Existing suite

Potential risks

None, feature

Documentation

Is hotfix candidate?

No

Shivs11 · 2025-01-28T13:48:57Z

service/matching/physical_task_queue_manager.go

-	// userData, _, err := c.partitionMgr.GetUserDataManager().GetUserData()
-	// if err != nil {
-	// 	return err
-	// }
-
-	// deploymentVersionData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentVersionData()
-	// if findDeploymentVersion(deploymentVersionData, workerDeployment.SeriesName, workerDeployment.BuildId) >= 0 {
-	// 	// already registered in user data, we can assume the workflow is running.
-	// 	// TODO: consider replication scenarios where user data is replicated before
-	// 	// the deployment workflow.
-	// 	return nil
-	// }


had forgotten to uncomment this and the next block of code - important since it basically serves as a de-dup mechanism for multiple polls coming in with the same worker-deployment information!

nice catch!

carlydf · 2025-01-28T18:48:56Z

service/frontend/workflow_handler.go

+		request.PageSize = maxPageSize
+	}
+
+	resp, nextPageToken, err := wh.workerDeploymentClient.ListWorkerDeployments(ctx, namespaceEntry, int(request.PageSize), request.NextPageToken)


I don't think it's worth changing for all of these APIs, but personally I find it easier to write code / keep up with shifting parameters if the input to my helper function (in this case workerDeploymentClient.ListWorkerDeployments) is just the proto request object + namespaceEntry. Then all the necessary parameters are already present.

Up to you though and I think not worth changing now, just letting you know for the future that it's totally fine to pass the whole request object if that's easier for you.

I think this was a design decision which was followed since the pre-release days and I didn't wanna alter it - fwiw, I agree with you - I shall keep things as is for now but not repeat the same thing in future

carlydf · 2025-01-28T18:50:43Z

service/matching/physical_task_queue_manager.go

-	// userData, _, err := c.partitionMgr.GetUserDataManager().GetUserData()
-	// if err != nil {
-	// 	return err
-	// }
-
-	// deploymentVersionData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentVersionData()
-	// if findDeploymentVersion(deploymentVersionData, workerDeployment.SeriesName, workerDeployment.BuildId) >= 0 {
-	// 	// already registered in user data, we can assume the workflow is running.
-	// 	// TODO: consider replication scenarios where user data is replicated before
-	// 	// the deployment workflow.
-	// 	return nil
-	// }


nice catch!

carlydf · 2025-01-28T18:55:13Z

service/matching/physical_task_queue_manager.go

+		deploymentVersions := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData()
+		if findDeploymentVersion(deploymentVersions, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 {
+			break


Suggested change

deploymentVersions := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData()

if findDeploymentVersion(deploymentVersions, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 {

break

deploymentData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData()

if findDeploymentVersion(deploymentData, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 {

break

This is still deploymentData, it just has a Deployments field and a Versions field now.

carlydf · 2025-01-28T18:55:49Z

service/matching/physical_task_queue_manager.go

+		select {
+		case <-userDataChanged:
+		case <-ctx.Done():
+			c.logger.Error("timed out waiting for deployment to appear in user data")


Suggested change

c.logger.Error("timed out waiting for deployment to appear in user data")

c.logger.Error("timed out waiting for worker deployment version to appear in user data")

Is this more accurate?

carlydf · 2025-01-28T18:59:15Z

service/worker/workerdeployment/client.go

@@ -539,7 +588,7 @@ func (d *ClientImpl) AddVersionToWorkerDeployment(
 	outcome, err := d.updateWithStart(
 		ctx,
 		namespaceEntry,
-		WorkerDeploymentVersionWorkflowType,
+		WorkerDeploymentWorkflowType,


nice catch -- although I'm confused why our tests didn't catch this. Seems like sending an update to the wrong WF type should have always failed

I know - I wondered on this too....

carlydf · 2025-01-28T19:00:46Z

service/worker/workerdeployment/util.go

@@ -44,7 +43,7 @@ const (
 	WorkerDeploymentWorkflowType        = "temporal-sys-worker-deployment-workflow"

 	// Namespace division
-	WorkerDeploymentVersionNamespaceDivision = "TemporalWorkerDeploymentVersion"
+	WorkerDeploymentVersionNamespaceDivision = "TemporalWorkerDeployment"


Should this variable be called WorkerDeploymentNamespaceDivision ?

I think the naming with "version" is left over from when "Deployment" (instance) was the "first-class-citizen" of this whole API

oops, yeah you are right - changing this to be called WorkerDeploymentNamespaceDivision

carlydf · 2025-01-28T19:03:45Z

service/worker/workerdeployment/version_workflow.go

-	// add version to worker-deployment workflow
-	activityCtx = workflow.WithActivityOptions(ctx, defaultActivityOptions)
-	err = workflow.ExecuteActivity(activityCtx, d.a.AddVersionToWorkerDeployment, &deploymentspb.AddVersionToWorkerDeploymentRequest{
-		DeploymentName: d.VersionState.DeploymentName,
-		Version:        d.VersionState.Version,
-		RequestId:      d.newUUID(ctx),
-	}).Get(ctx, nil)
-	if err != nil {
-		return err
-	}
-


why is this deleted?

ah sorry - we were adding the same version twice to the worker-deployment workflow (we were doing this twice)!

oh I see the duplicate code above -- got it

carlydf · 2025-01-28T19:12:25Z

service/worker/workerdeployment/workflow.go

@@ -115,7 +117,7 @@ func (d *WorkflowRunner) run(ctx workflow.Context) error {
 }

 func (d *WorkflowRunner) validateSetCurrent(args *deploymentspb.SetCurrentVersionArgs) error {
-	if d.State.CurrentVersion != args.Version {
+	if d.State.RoutingInfo.CurrentVersion != args.Version {


Nit: I think it's more idiomatic in Go, and more readable, to structure the function like

func foo(...) error { if errorCondition { return err } return nil }

Returning success at the end and errors in the middle makes it easier to read. The section "A longer example" in https://8thlight.com/insights/exploring-error-handling-patterns-in-go this article also calls this out with another example.

## What changed?  - ListWorkerDeployments API - Also removed presence of locks when adding a version to a worker-deployment workflow. Causes deadlocks and we don't need them. ## Why?  - Versioning-3.1 ## How did you test it?  - Added functional tests (happy-path) - Existing suite ## Potential risks  - None, feature ## Documentation  ## Is hotfix candidate?  - No

Shivs11 added 4 commits January 27, 2025 17:43

current work on adding list worker deployment

e33a4d5

tests + working prototype of ListWorkerDeployments

274e96c

removed locking from handleAddVersionToWorkerDeployment

732cd66

removing stale comment

4ac7fdd

Shivs11 commented Jan 28, 2025

View reviewed changes

Shivs11 added 3 commits January 28, 2025 08:50

restored state of an old file

1a02c7d

removed stale print

9ae9d85

lint fixes + increase max duration between timestamps

ee7da1e

Shivs11 marked this pull request as ready for review January 28, 2025 14:50

Shivs11 requested a review from a team as a code owner January 28, 2025 14:50

carlydf reviewed Jan 28, 2025

View reviewed changes

addressed comments

e0c9521

carlydf approved these changes Jan 28, 2025

View reviewed changes

Shivs11 merged commit a1fc7bd into versioning-3.1 Jan 28, 2025
41 of 49 checks passed

Shivs11 deleted the ss/listworkerdeployment-versioning3.1 branch January 28, 2025 20:07

	c.logger.Error("timed out waiting for deployment to appear in user data")
	c.logger.Error("timed out waiting for worker deployment version to appear in user data")

ListWorkerDeployments : Versioning-3.1 #7173

ListWorkerDeployments : Versioning-3.1 #7173

Uh oh!

Conversation

Shivs11 commented Jan 28, 2025

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!