Use string values for deployment version by ShahabT · Pull Request #547 · temporalio/api

ShahabT · 2025-02-03T06:28:08Z

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?

Using string type in all the places for version fields. Removed WorkerDeploymentVersion from the public API. This is for two reasons:
- String type gives ability to specify special values of __unversioned__ (and maybe later <deployment_name>/__unversioned__.
- We might add ability for users to pass custom version IDs in with case the struct would be hard for users to use (they'd need to write the logic to convert the struct to an ID based on whether it has a custom ID or not).
Using WorkerDeploymentOptions in the task response APIs instead of WorkerDeploymentVersion so that server has the full info regardless of the worker participating in versioning or not.
Added worker_deployment field to execution info (top level) so both versioned and unversioned workflows can have this info (and its corresponding SA).
Renamed WorkflowVersioningMode to WorkerVersioningMode and its VERSIONING_BEHAVIORS enum to VERSIONED, for the following reasons:
- This enum is more about the worker than the workflows, whether or not workflows are versioned or not depends on whether or not the worker enables versioning.
- We tried to not use the terms "versioned" and "unversioned" for workers for a while but the attempt did work and we ended up even creating special __unversioned__ value for them. Hence, it'd be very confusing if they don't see some config such as WorkerVersioningMode that directly specifies the worker as versioned or unversioned. The previous name, while doing the same thing, twisted the naming and I believe it'll confuse the users.
- Deployment Versions are not created for the unversioned mode, at least not so far. This naming and updated comments reflects that better.
- WorkflowVersioningMode is easy to be confused with Workflow VersioningBehavior.

Why?

Breaking changes

Server PR

temporal/api/deployment/v1/message.proto

temporal/api/enums/v1/deployment.proto

cretz

LGTM since going to feature branch and doesn't appear to be affecting any master APIs

ShahabT

Change requests that came out of in-person meeting with @antlai-temporal @carlydf @Shivs11 .

ShahabT · 2025-02-03T17:44:59Z

temporal/api/deployment/v1/message.proto

+    // same `deployment_name` and `build_id`, across all Task Queues.
+    // When `versioning_mode==VERSIONED`, the worker will be part of a Deployment Version identified
+    // by "<deployment_name>/<build_id>".
+    temporal.api.enums.v1.WorkerVersioningMode versioning_mode = 3;


-> worker_versioning_mode

temporal/api/deployment/v1/message.proto

ShahabT · 2025-02-03T17:46:01Z

temporal/api/history/v1/message.proto

    // `versioning_behavior` is set. This value updates workflow execution's
    // `versioning_info.deployment_version`.
-    temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 9;
+    string deployment_version = 9;


add format comment

add deployment name too

ShahabT · 2025-02-03T17:56:06Z

temporal/api/workflow/v1/message.proto

    WorkflowExecutionVersioningInfo versioning_info = 22;
+
+    // The name of Worker Deployment that completed the most recent workflow task.
+    string worker_deployment = 23;


-> worker_deployment_name

ShahabT · 2025-02-03T17:59:07Z

temporal/api/workflow/v1/message.proto

    // Note that if `versioning_override.behavior` is PINNED then `versioning_override.pinned_version`
    // will override this value.
-    temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 5;
+    string deployment_version = 5;


ShahabT · 2025-02-03T17:59:13Z

temporal/api/workflow/v1/message.proto

-    temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 1;
+    // Required. The target version of the transition. May be `__unversioned__` which means a
+    // so-far-versioned workflow is transitioning to unversioned workers.
+    string deployment_version = 1;


temporal/api/workflow/v1/message.proto

ShahabT · 2025-02-03T18:08:53Z

temporal/api/workflowservice/v1/request_response.proto

+//  - It is not the Current or Ramping Version of its Deployment.
 //  - It has no active pollers (none of the task queues in the Version have pollers)
-//  - It is drained (see WorkerDeploymentVersionInfo.drainage_info)
+//  - It is drained (see WorkerDeploymentVersionInfo.drainage_info). This condition can be skipped


Make it clear that nil drainage info is ok

temporal/api/deployment/v1/message.proto

temporal/api/enums/v1/deployment.proto

carlydf · 2025-02-03T19:26:44Z

temporal/api/enums/v1/deployment.proto

+    // Workers with this mode are part of a Worker Deployment Version which is identified as
+    // "<deployment_name>/<build_id>". Such workers are called "versioned" as opposed to
+    // "unversioned".
+    // Each Deployment Version is distinguished from other Versions and users can configure Temporal


Suggested change

// Each Deployment Version is distinguished from other Versions and users can configure Temporal

// Each Deployment Version is unique within a Namespace and users can configure the Temporal

I meant they are distinguished for routing task. I'll make that clear.

temporal/api/enums/v1/deployment.proto

carlydf · 2025-02-03T19:45:05Z

temporal/api/workflowservice/v1/request_response.proto

+    // Optional. By default this request would be rejected if not all the expected Task Queues are
+    // being polled by the new Version, to protects against accidental removal of Task Queues, or
+    // worker health issues. Pass `true` here to bypass this protection.
+    // The set of expected Task Queues equals to all the Task Queues ever polled from the existing


Suggested change

// The set of expected Task Queues equals to all the Task Queues ever polled from the existing

// The set of expected Task Queues is the set of all the Task Queues that ever belonged to the existing

carlydf · 2025-02-03T19:54:28Z

temporal/api/workflowservice/v1/request_response.proto

+    //     add_rate of 0.)
+    //   - Task Queues that are moved to another Worker Deployment (inferred by the Task Queue
+    //     having a different Current Version than the Current Version of this deployment.)
+    // WARNING: Do not set this flag unless you are sure that the missing task queue poller are not


Suggested change

// WARNING: Do not set this flag unless you are sure that the missing task queue poller are not

// WARNING: Do not set this flag unless you are sure that the missing task queue pollers are not

carlydf · 2025-02-03T19:54:51Z

temporal/api/workflowservice/v1/request_response.proto

+    //     having a different Current Version than the Current Version of this deployment.)
+    // WARNING: Do not set this flag unless you are sure that the missing task queue poller are not
+    // needed. If the request is unexpectedly rejected due to missing pollers, then that means the
+    // pollers have not reached to server yet.


Suggested change

// pollers have not reached to server yet.

// pollers have not reached the server yet.

carlydf · 2025-02-03T19:56:03Z

temporal/api/workflowservice/v1/request_response.proto

+    string namespace = 1;
+    // Deployment Version identifier in the form "<deployment_name>/<build_id>".
+    string version = 2;
+    // Pass to force deletion even if the Version is not drained. In this case the open pinned


Suggested change

// Pass to force deletion even if the Version is not drained. In this case the open pinned

// Pass to force deletion even if the Version is draining. In this case the open pinned

carlydf · 2025-02-03T19:56:48Z

temporal/api/workflowservice/v1/request_response.proto

+//  - It is not the Current or Ramping Version of its Deployment.
 //  - It has no active pollers (none of the task queues in the Version have pollers)
-//  - It is drained (see WorkerDeploymentVersionInfo.drainage_info)
+//  - It is not in Draining status (see WorkerDeploymentVersionInfo.drainage_info). This condition


Suggested change

// - It is not in Draining status (see WorkerDeploymentVersionInfo.drainage_info). This condition

// - It is not draining (see WorkerDeploymentVersionInfo.drainage_info). This condition

Co-authored-by: Carly de Frondeville <[email protected]>

…ld-id

## What changed?  Updated code to use latest API changes in which Deployment Versions are represented by string fields rather than structs. `WorkerDeploymentVersion` proto message still exists but only for the internal APIs. ## Why?  See temporalio/api#547. ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Carly de Frondeville <[email protected]>

Use string values for deployment version

1d8affa

ShahabT requested review from Shivs11, antlai-temporal and carlydf February 3, 2025 06:28

ShahabT requested review from a team as code owners February 3, 2025 06:28

ShahabT mentioned this pull request Feb 3, 2025

Clarify semantics of TaskQueueVersioningInfo current_version and ramping_version #545

Closed

Add skip-drainage flag for DeleteVersion

f568e3a

ShahabT mentioned this pull request Feb 3, 2025

Use Deployment Version strings in all user-facing APIs temporalio/temporal#7219

Merged

Shivs11 reviewed Feb 3, 2025

View reviewed changes

temporal/api/deployment/v1/message.proto Show resolved Hide resolved

Shivs11 reviewed Feb 3, 2025

View reviewed changes

temporal/api/enums/v1/deployment.proto Outdated Show resolved Hide resolved

Shivs11 approved these changes Feb 3, 2025

View reviewed changes

cretz approved these changes Feb 3, 2025

View reviewed changes

ShahabT commented Feb 3, 2025

View reviewed changes

ShahabT added 2 commits February 3, 2025 11:30

address comments

8f16501

address comments

821a99a

carlydf reviewed Feb 3, 2025

View reviewed changes

ShahabT and others added 3 commits February 3, 2025 12:26

Apply suggestions from code review

b1ec4f5

Co-authored-by: Carly de Frondeville <[email protected]>

Merge remote-tracking branch 'origin/shahab/build-id' into shahab/bui…

022a853

…ld-id

address comments

7769dc5

ShahabT merged commit 85f6dce into versioning-3.1 Feb 3, 2025

ShahabT deleted the shahab/build-id branch February 3, 2025 20:56

	// Each Deployment Version is distinguished from other Versions and users can configure Temporal
	// Each Deployment Version is unique within a Namespace and users can configure the Temporal

	// The set of expected Task Queues equals to all the Task Queues ever polled from the existing
	// The set of expected Task Queues is the set of all the Task Queues that ever belonged to the existing

	// WARNING: Do not set this flag unless you are sure that the missing task queue poller are not
	// WARNING: Do not set this flag unless you are sure that the missing task queue pollers are not

	// pollers have not reached to server yet.
	// pollers have not reached the server yet.

	// Pass to force deletion even if the Version is not drained. In this case the open pinned
	// Pass to force deletion even if the Version is draining. In this case the open pinned

	// - It is not in Draining status (see WorkerDeploymentVersionInfo.drainage_info). This condition
	// - It is not draining (see WorkerDeploymentVersionInfo.drainage_info). This condition

Conversation

ShahabT commented Feb 3, 2025

Uh oh!

Uh oh!

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

ShahabT left a comment

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!

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!

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants