Skip to content

Use fully-qualified version string (deployment_name/build_id) for routing APIs#544

Merged
carlydf merged 2 commits intoversioning-3.1from
cdf/future-proof-for-byok
Feb 2, 2025
Merged

Use fully-qualified version string (deployment_name/build_id) for routing APIs#544
carlydf merged 2 commits intoversioning-3.1from
cdf/future-proof-for-byok

Conversation

@carlydf
Copy link
Copy Markdown
Contributor

@carlydf carlydf commented Feb 2, 2025

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.

Switch string build_id to string version in SetCurrent and SetRamp.
Explain that in RoutingInfo the current_version and ramping_version strings will be the fully-qualified version string instead of just build id.

btw, I'm assuming that when we add BYOK, we will simply add to WorkerDeploymentOptions and WorkerDeploymentVersion, so I'm not renaming build id there.
Right now they are:

// Identifies a Worker Deployment Version. The combination of `deployment_name` and `build_id`
// serve as the identifier.
message WorkerDeploymentVersion {
    // The name of the Deployment this version belongs too.
    string deployment_name = 1;
    // Build ID uniquely identifies the Deployment Version within a Deployment, but the same Build
    // ID can be used in multiple Deployments.
    string build_id = 2;
}

// Worker Deployment options set in SDK that need to be sent to server in every poll.
message WorkerDeploymentOptions {
    // Required. Worker Deployment name.
    string deployment_name = 1;
    // Required. Build ID of the worker. `deployment_name` and `build_id` together identify this
    // Worker Deployment Version.
    string build_id = 2;
    // Required. Workflow versioning mode for this Deployment Version. Must be the same for all
    // pollers in a given Deployment Version, across all Task Queues.
    temporal.api.enums.v1.WorkflowVersioningMode workflow_versioning_mode = 3;
}

Not trying to design this now, just trying to validate that the current structure is BYOK-compatible -- I'm imagining that in the future they could look something like:

+ // Identifies a Worker Deployment Version, which is identified by either:
+ //    - The combination of `deployment_name` and `build_id`.
+ //    - If present, `id`. 
message WorkerDeploymentVersion {
    // The name of the Deployment this version belongs too.
    string deployment_name = 1;
    // Build ID uniquely identifies the Deployment Version within a Deployment, but the same Build
    // ID can be used in multiple Deployments.
    string build_id = 2;
+    // Uniquely identifies the Deployment Version across a Namespace.
+    string id = 3;
}

// Worker Deployment options set in SDK that need to be sent to server in every poll.
message WorkerDeploymentOptions {
+    // Required unless `version_id` is present. Worker Deployment name.
    string deployment_name = 1;
+    // Required unless `version_id` is present. Build ID of the worker. `deployment_name` and 
+    // `build_id` together identify this Worker Deployment Version.
    string build_id = 2;
    // Required. Workflow versioning mode for this Deployment Version. Must be the same for all
    // pollers in a given Deployment Version, across all Task Queues.
    temporal.api.enums.v1.WorkflowVersioningMode workflow_versioning_mode = 3;
+    // Required only if `deployment_name` or `build_id` are not present.
+    // Uniquely identifies this Worker Deployment Version in the namespace.
+    string version_id 4;
}

Why?

Breaking changes

Server PR

@carlydf carlydf requested review from a team as code owners February 2, 2025 04:59
@carlydf carlydf merged commit 082e064 into versioning-3.1 Feb 2, 2025
@carlydf carlydf deleted the cdf/future-proof-for-byok branch February 2, 2025 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants