Skip to content

Conversation

carlydf
Copy link
Contributor

@carlydf carlydf commented Apr 26, 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.

What changed?

  • Make override a one of and add different pinned override types.
  • Change version from string to struct
  • Make nil Version represent "send tasks away from versioned workers and to unversioned workers"

Why?

  • To prevent misconfiguration and to expand the override definition to accommodate the new PinnedUntilContinueAsNew behaviors.
  • Using a string to represent structured data, whose subfields frequently need to be considered separately, is brittle and not good for anyone using it. Struct is better.
  • "unversioned version" is contradictory, and nil is a nice way to represent that the versioning story of a task queue is no longer in the control of the Deployment whose RoutingConfig you are reading.

Variables are being deprecated, but the content of the code is not changing.
The idea of "Unset Ramp" having a separate meaning from "0% Ramp" is removed. 0% ramp now is the only way to have "no ramp" / 100% of traffic going to Current Version. Nil ramping_deployment_version + non-zero ramping_version_percentage is the way to ramp traffic to unversioned workers.

The SDK interface for this could look like:

deployment := client.GetHandle(deploymentName)

// set current to a version
deployment.SetCurrentVersion(buildId) -> current set to deploymentName/buildId
deployment.UnsetCurrentVersion() -> current set to nil

// set current to unversioned
deployment.UnsetCurrentVersion() -> current set to nil


deployment.SetRampingVersion(buildId, 50) -> ramp set to (deploymentName/buildId, 50)
deployment.UnsetRampingVersion() -> ramp set to (nil, 0%)
deployment.RampToUnversioned(50) -> set to (nil, 50%)

Server PR

@carlydf carlydf requested review from a team as code owners April 26, 2025 00:03
@carlydf carlydf changed the title prevent misconfiguration of VersioningOverride and add more pinned options change Version to struct, prevent misconfiguration of VersioningOverride Apr 29, 2025
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Much safer. Having all these deprecated fields is slightly sad, but, that's life. Let's make sure we track actually removing them after things are GA

None of my comments are blocking.

@carlydf carlydf changed the base branch from versioning-3.2 to master May 1, 2025 23:47
@carlydf carlydf force-pushed the versioning-override branch from 2ebd0b3 to 9ab5def Compare May 2, 2025 18:39
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, happy with this and 10x more obvious than the previous iteration

@carlydf carlydf merged commit 9263046 into master May 13, 2025
5 checks passed
@carlydf carlydf deleted the versioning-override branch May 13, 2025 20:24
mjameswh added a commit that referenced this pull request May 15, 2025
**What changed?**

- Fix a double semicolon introduced in #579.

**Why?**

- The double semicolon sequence is causing a compilation error in
`protobufjs`, even though this is technically valid according to [the
official Protobuf 3
spec](https://protobuf.dev/reference/protobuf/proto3-spec/#emptystatement).

This is a bug in `protobufjs`, for which there's been [an open issue
ticket](protobufjs/protobuf.js#1322) since
2019, but it was never acknowledged, and a PR fixing this has been
ignored for more than a year.

Given that there's simply no reason to use a double semicolon sequence
anyway, it appears preferable to simply avoid that in our proto
definitions.
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.

5 participants