RegisterTaskQueue + DescribeVersion: versioning-3.1 #7107

Shivs11 · 2025-01-17T16:35:22Z

What changed?

Updated deployment entity wf's API's (naming) for versioning-3.1. Removed those which will not be required.
Added functionality for RegisterTaskQueue + DescribeVersion
Added functional tests for verifying the above two work.
UserData stuff has been removed right now since there are pending discussions

Why?

Versioning-3.1

How did you test it?

Functional tests have been added. Note, the tests added are exact replicas (in terms of their core functionality) with the existing tests under deployment_test.go

Potential risks

None, feature branch.

Documentation

Is hotfix candidate?

No

Shivs11 · 2025-01-17T16:36:15Z

proto/internal/temporal/server/api/deployment/v1/message.proto

@@ -34,7 +34,8 @@ import "temporal/api/common/v1/message.proto";
 // Data for each worker deployment version + task queue pair.
 // This is stored in each worker deployment version (for each task queue),
 // and synced to task queue user data (for each worker deployment version).
-message WorkerDeploymentTaskQueueData {


@carlydf - I thought these names were a tad bit better while using them
lmk your thoughts

makes sense to me since the task queues are more tied to the deployment version (instance) than the worker deployment (group)!

Shivs11 · 2025-01-17T16:36:44Z

service/frontend/workflow_handler.go

@@ -3306,6 +3314,42 @@ func (wh *WorkflowHandler) SetCurrentDeployment(ctx context.Context, request *wo
 	}, nil
 }

+// Versioning-3 Public-Preview API's
+
+func (wh *WorkflowHandler) DescribeWorkerDeploymentVersion(ctx context.Context, request *workflowservice.DescribeWorkerDeploymentVersionRequest) (_ *workflowservice.DescribeWorkerDeploymentVersionResponse, retError error) {


functionality copied from existing working API for deployments: DescribeDeployment

service/matching/physical_task_queue_manager.go

Shivs11 · 2025-01-17T16:41:20Z

service/matching/physical_task_queue_manager.go

+		return c.deploymentRegisterError
+	}
+
+	// userData, _, err := c.partitionMgr.GetUserDataManager().GetUserData()


UserData stuff will come in later since there are pending discussions on how to finalize this structure.

ShahabT · 2025-01-17T17:16:38Z

proto/internal/temporal/server/api/matchingservice/v1/request_response.proto

@@ -328,7 +328,7 @@ message SyncDeploymentUserDataRequest {
    temporal.api.enums.v1.TaskQueueType task_queue_type = 3;

    // This is the deployment being modified.
-    temporal.api.deployment.v1.Deployment deployment = 4;
+    temporal.api.deployment.v1.Deployment deployment = 4; 


Nit: extra space

ShahabT

overall looks good.

service/matching/physical_task_queue_manager.go

service/worker/workerdeployment/fx.go

carlydf · 2025-01-17T19:44:35Z

common/dynamicconfig/constants.go

@@ -857,11 +857,19 @@ of Timeout and if no activity is seen even after that the connection is closed.`
 		true,
 		`FrontendEnableSchedules enables schedule-related RPCs in the frontend`,
 	)
+	// [cleanup-wv-pre-release]
 	EnableDeployments = NewNamespaceBoolSetting(
 		"system.enableDeployments",
 		false,
 		`EnableDeployments enables deployments (versioning v3) in all services,


Suggested change

`EnableDeployments enables deployments (versioning v3) in all services,

`EnableDeployments enables deployments (deprecated versioning v3 pre-release) in all services,

up to you but I'm thinking this is good so that whatever engineer who is asked to "enabled deployments" by customer service does not accidentally enable the old deployments (they may not know what the // cleanup comment means)

carlydf · 2025-01-17T19:47:10Z

proto/internal/temporal/server/api/deployment/v1/message.proto

@@ -34,7 +34,8 @@ import "temporal/api/common/v1/message.proto";
 // Data for each worker deployment version + task queue pair.
 // This is stored in each worker deployment version (for each task queue),
 // and synced to task queue user data (for each worker deployment version).
-message WorkerDeploymentTaskQueueData {


makes sense to me since the task queues are more tied to the deployment version (instance) than the worker deployment (group)!

carlydf · 2025-01-17T20:08:23Z

service/matching/physical_task_queue_manager.go

+	// lock so that only one poll does the update and the rest wait for it
+	c.deploymentLock.Lock()
+	defer c.deploymentLock.Unlock()


is this code just copied for now? are you thinking to use the same lock to cover updates to both the old and the new deployment entity WFs, or two separate locks? (same lock might be the right choice, I'm not sure, just want to discuss)

good question - I had thought about that and I ended up choosing to keep the locks the same since these locks just make sure that only one update happens at the same time - moreover, both the entity wf updates happen sequentially so I don't see the use-case of using two separate locks instead of just one (naming of the locks will change later though)

service/worker/pernamespaceworker.go

service/worker/workerdeployment/client.go

service/worker/workerdeployment/util.go

carlydf

approved once comments are addressed

…aking unit-tests

common/dynamicconfig/constants.go

ShahabT · 2025-01-18T00:59:20Z

service/worker/workerdeployment/fx.go

 		},
 	}
 }

 func (s *workerComponent) DedicatedWorkerOptions(ns *namespace.Namespace) *workercommon.PerNSDedicatedWorkerOptions {
 	return &workercommon.PerNSDedicatedWorkerOptions{
-		Enabled: s.enabledForNs(ns.Name().String()),
+		Enabled: s.enabled(),


Suggested change

Enabled: s.enabled(),

Enabled: true,

ShahabT · 2025-01-18T00:59:34Z

service/worker/workerdeployment/fx.go

@@ -41,7 +41,7 @@ import (
 type (
 	workerComponent struct {
 		activityDeps activityDeps
-		enabledForNs dynamicconfig.BoolPropertyFnWithNamespaceFilter
+		enabled      dynamicconfig.BoolPropertyFn


no need for this field.

…r a ns

## What changed?  - Updated deployment entity wf's API's (naming) for versioning-3.1. Removed those which will not be required. - Added functionality for RegisterTaskQueue + DescribeVersion - Added functional tests for verifying the above two work. - UserData stuff has been removed right now since there are pending discussions ## Why?  - Versioning-3.1 ## How did you test it?  - Functional tests have been added. Note, the tests added are exact replicas (in terms of their core functionality) with the existing tests under `deployment_test.go` ## Potential risks  - None, feature branch. ## Documentation  ## Is hotfix candidate?  - No

Shivs11 added 3 commits January 16, 2025 15:53

renaming deployment entity wfs

bf85205

fixed cherry-picking conflicts

047bb53

RegisterTaskQueue + DescribeVersion - versioning-3.1

459e119

Shivs11 commented Jan 17, 2025

View reviewed changes

service/matching/physical_task_queue_manager.go Show resolved Hide resolved

Shivs11 commented Jan 17, 2025

View reviewed changes

lint fixes

89a3bf6

Shivs11 requested review from ShahabT and carlydf January 17, 2025 16:46

Shivs11 marked this pull request as ready for review January 17, 2025 16:51

Shivs11 requested a review from a team as a code owner January 17, 2025 16:51

ShahabT reviewed Jan 17, 2025

View reviewed changes

service/matching/physical_task_queue_manager.go Show resolved Hide resolved

service/worker/workerdeployment/fx.go Outdated Show resolved Hide resolved

Enabling deployment version config for all namespaces

96e12ca

carlydf reviewed Jan 17, 2025

View reviewed changes

carlydf approved these changes Jan 17, 2025

View reviewed changes

updated proto name according to latest naming conventions + fixed bre…

a205d68

…aking unit-tests

Shivs11 requested a review from ShahabT January 17, 2025 22:49

ShahabT reviewed Jan 18, 2025

View reviewed changes

common/dynamicconfig/constants.go Outdated Show resolved Hide resolved

ShahabT reviewed Jan 18, 2025

View reviewed changes

worker component enabled everytime deployment versions are enabled fo…

9be3dda

…r a ns

Shivs11 requested a review from ShahabT January 20, 2025 14:26

Shivs11 merged commit 957bc26 into versioning-3.1 Jan 20, 2025
49 checks passed

Shivs11 deleted the ss/deployment-wf-v-3.1 branch January 20, 2025 16:15

	`EnableDeployments enables deployments (versioning v3) in all services,
	`EnableDeployments enables deployments (deprecated versioning v3 pre-release) in all services,

RegisterTaskQueue + DescribeVersion: versioning-3.1 #7107

RegisterTaskQueue + DescribeVersion: versioning-3.1 #7107

Uh oh!

Conversation

Shivs11 commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShahabT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Shivs11 commented Jan 17, 2025 •

edited

Loading