Support Worker Deployments 3.1 #1814

antlai-temporal · 2025-02-11T08:34:14Z

What was changed

This PR introduces Worker Deployments using the versioning 3.1 API

Systems tests need the new CLI to pass but they are almost complete.

Checklist

Closes
Rename Worker Deployment API interfaces #1778 Add ramp to Worker Deployments #1777
How was this tested:

unit tests, system tests...

client/client.go

Quinn-With-Two-Ns · 2025-02-11T16:02:47Z

client/client.go

 type DeploymentReachability = internal.DeploymentReachability

 const (
 	// DeploymentReachabilityUnspecified - Reachability level not specified.
-	// NOTE: Experimental
+	// Deprecated: Use [WorkerDeploymentVersionDrainageStatus]


Were these APIs released as Public Preview? I am just wondering why we are deprecating instead of just removing?

No, only pre-release. I think the plan was to give a heads up with one release deprecated and remove them in the next one for public preview.

Discussing with @ShahabT it seems that no customers have been enabled in Cloud, and it has been out for just 2 months. If you feel strongly about removing it, I can bring back the issue with the team...

I don't mind if they stay for a short bit, but yes I think we should consider removing soon

Yeah as long as we aren't compromising on the API names because the best names are taken by the old API I am fine to leave the old API for a short bit. Can we commit to removing the old API for the next minor release after the new API is released? so v1.34.0.

Sounds good to me. The prefix Worker_Deployment vs Deployment seems to work fine for the API names, and avoids the confusion with other deployments...

internal/worker.go

Quinn-With-Two-Ns · 2025-02-11T16:11:19Z

internal/internal_nexus_task_poller.go

@@ -96,6 +97,10 @@ func (ntp *nexusTaskPoller) poll(ctx context.Context) (taskForWorker, error) {
 			UseVersioning:        ntp.useBuildIDVersioning,
 			DeploymentSeriesName: ntp.deploymentSeriesName,
 		},
+		DeploymentOptions: workerDeploymentOptionsToProto(


Just double checking deployments do work for Nexus tasks right?

It should dispatch nexus tasks based on current/ramping versions. No testing so far though...

cretz

Only reviewed the public API, most seems fine, only minor comments

cretz · 2025-02-14T15:40:15Z

client/client.go

 type DeploymentReachability = internal.DeploymentReachability

 const (
 	// DeploymentReachabilityUnspecified - Reachability level not specified.
-	// NOTE: Experimental
+	// Deprecated: Use [WorkerDeploymentVersionDrainageStatus]


I don't mind if they stay for a short bit, but yes I think we should consider removing soon

internal/worker_deployment_client.go

cretz · 2025-02-14T15:47:24Z

internal/worker_deployment_client.go

+	// NOTE: Experimental
+	//
+	// Exposed as: [go.temporal.io/sdk/client.WorkerDeploymentConflictToken]
+	WorkerDeploymentConflictToken struct {


Hrmm, not too sure there is value in a struct wrapper for this byte slice. Why not just pass the byte slice around (it's normal for a zero-len (aka nil) byte slice to be the equivalent of unset, you don't need struct wrapper + pointer).

We had this discussion last time, but here token is optional, and that forced me to use a pointer. So I think you are right here, moving to empty slice as nil will simplify the API...

An optional byte slice in Go is very normal and is nil/empty (but you use operations like len == 0 regardless to determine unset)

cretz · 2025-02-14T15:59:24Z

internal/worker_deployment_client.go

+		// with that name in this namespace, methods like WorkerDeploymentHandle.Describe()
+		// will return an error.
+		// NOTE: Experimental
+		GetHandle(ctx context.Context, name string) WorkerDeploymentHandle


How is a worker deployment created? If it's done lazily on one of the setters, I am concerned about how conflict version works because I am not sure how a user can say "create only if not created" in a race-safe way.

The SDK does not create deployments, the server creates them as a side-effect of seeing pollers annotated for a new deployment when they poll. GetHandle does not do much, just local...

So can you confirm every call on a WorkflowDeploymentHandle for a workflow deployment that does not exist returns a not-found? Meaning none of these calls work without a server having implicitly created such a deployment? Is it not a bit confusing that you cannot do some worker deployment preparation/configuration without having started a poller on it first? That's a really confusing way to create a worker deployment.

Yes, I don't like the "eventually consistent" approach for deployments/versions since poller existence (or not) changes policy, and before it was independent of pollers, but that's fundamental to how versioning 3.1 works... On the positive side, it can be easier to use, just annotate workers and server does the rest...

internal/worker_deployment_client.go

…ployment-api

Quinn-With-Two-Ns · 2025-02-18T18:34:45Z

.github/workflows/ci.yml

@@ -87,6 +87,9 @@ jobs:
      - name: Docker compose - integration tests
        if: ${{ matrix.testDockerCompose }}
        run: go run . integration-test
+        env:
+          # TODO(antlai-temporal): Remove this flag once server 1.27 released.
+          DISABLE_WORKER_DEPLOYMENT_TESTS: "1"


Suggested change

DISABLE_WORKER_DEPLOYMENT_TESTS: "1"

DISABLE_SERVER_1_27_TESTS: "1"

@rodrigozhou will also need this flag to test attaching nexus callbacks I believe

Adding the flag

Support Worker Deployments 3.1

337509e

Quinn-With-Two-Ns reviewed Feb 11, 2025

View reviewed changes

client/client.go Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Feb 11, 2025

View reviewed changes

internal/worker.go Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Feb 11, 2025

View reviewed changes

antlai-temporal added 3 commits February 13, 2025 18:38

Adding more tests

3c5fe3c

Increase delay

3ac930b

Add system tests

dec455b

antlai-temporal marked this pull request as ready for review February 14, 2025 10:36

antlai-temporal requested a review from a team as a code owner February 14, 2025 10:36

cretz reviewed Feb 14, 2025

View reviewed changes

Simplify conflict token, GetHandle, and provide identity default

d62d574

antlai-temporal mentioned this pull request Feb 18, 2025

doclink: Incorrect "Exposed" requirement in method without context #1829

Open

antlai-temporal added 5 commits February 17, 2025 20:39

Bypass doclink bug

b7de2b5

Fix comments

9bfd670

Merge remote-tracking branch 'upstream/versioning-3.1' into rename-de…

e136450

…ployment-api

Merge remote-tracking branch 'upstream/versioning-3.1' into rename-de…

51e18e2

…ployment-api

Fix deprecated comment

fd56dd9

cretz approved these changes Feb 18, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed Feb 18, 2025

View reviewed changes

Adding CI flag

866bff4

antlai-temporal merged commit 7613853 into temporalio:versioning-3.1 Feb 18, 2025
3 of 11 checks passed

antlai-temporal mentioned this pull request Feb 18, 2025

Support Worker Deployments 3.1 #1832

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Worker Deployments 3.1 #1814

Support Worker Deployments 3.1 #1814

antlai-temporal commented Feb 11, 2025 •

edited

Loading

Quinn-With-Two-Ns Feb 11, 2025

antlai-temporal Feb 11, 2025

antlai-temporal Feb 11, 2025

cretz Feb 14, 2025

Quinn-With-Two-Ns Feb 18, 2025

antlai-temporal Feb 18, 2025

Quinn-With-Two-Ns Feb 11, 2025

antlai-temporal Feb 11, 2025

cretz left a comment

cretz Feb 14, 2025

cretz Feb 14, 2025

antlai-temporal Feb 18, 2025

cretz Feb 18, 2025

cretz Feb 14, 2025

antlai-temporal Feb 14, 2025

cretz Feb 14, 2025 •

edited

Loading

antlai-temporal Feb 14, 2025

Quinn-With-Two-Ns Feb 18, 2025

antlai-temporal Feb 18, 2025

	DISABLE_WORKER_DEPLOYMENT_TESTS: "1"
	DISABLE_SERVER_1_27_TESTS: "1"

Support Worker Deployments 3.1 #1814

Support Worker Deployments 3.1 #1814

Conversation

antlai-temporal commented Feb 11, 2025 • edited Loading

What was changed

Checklist

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

cretz left a 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

cretz Feb 14, 2025 • edited Loading

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

antlai-temporal commented Feb 11, 2025 •

edited

Loading

cretz Feb 14, 2025 •

edited

Loading