-
Notifications
You must be signed in to change notification settings - Fork 69
change Version to struct, prevent misconfiguration of VersioningOverride #579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
Co-authored-by: Spencer Judge <[email protected]>
…versioning-override
// Not yet supported. Reserved for future use. | ||
// Override workflow behavior to be Pinned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is gonna be supported (as part of the first change using this API), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was to only support the default UNSPECIFIED
behavior in this first change, which means
// If original behavior is Pinned or PinnedUntilContinueAsNew, keep that behavior.
// If original behavior is AutoUpgrade, override behavior to Pinned.
because I felt that that maps best to what we currently have (in the world without 2 different types of pinned).
but arguably, // Override workflow behavior to be Pinned.
(in the new world) is more equivalent to "pinned override" in the old world..
I'm fine with either, but "old-world pinned override" should default to exactly one of these three types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a pure structural POV, but will want to wait for @Sushisource approval. It is unfortunate to have so many versioning deprecations overall on the API, I wonder if there are preventative things we can do in the future on other projects.
2ebd0b3
to
9ab5def
Compare
There was a problem hiding this 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
Co-authored-by: Shahab Tajik <[email protected]>
Co-authored-by: Shahab Tajik <[email protected]>
…ersion and deployment options
@@ -374,8 +392,11 @@ message ActivityTaskCompletedEventAttributes { | |||
// id of the worker that completed this task | |||
string identity = 4; | |||
// Version info of the worker who processed this workflow task. | |||
// Deprecated. Use the info inside the corresponding ActivityTaskStartedEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShahabT There is no more versioning info in the ActivityTaskStartedEvent, since we deprecated the version stamp and didn't replace it with anything.
We can ask the worker that is calling ActivityTaskCompleted
to send a deployment options, and then turn that into a version, or we could do nothing. AFAIK, we store in the workflow execution info . versioning info the version that completed the last workflow task, but we do not store the version that completed the last activity task. So I forget how we handle versioning of activities, or if we even need to know what worker completed the activity task. But either way the comment "Use the info inside the corresponding ActivityTaskStartedEvent" needs to be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a last_deployment_version
in PendingActivityInfo
// The Worker Deployment Version this activity was dispatched to most recently.
// If nil, the activity has not yet been dispatched or was last dispatched to an unversioned worker.
temporal.api.deployment.v1.WorkerDeploymentVersion last_deployment_version = 24;
I'm not yet sure when it's populated or what it's used for
@@ -389,8 +410,11 @@ message ActivityTaskFailedEventAttributes { | |||
string identity = 4; | |||
temporal.api.enums.v1.RetryState retry_state = 5; | |||
// Version info of the worker who processed this workflow task. | |||
// Deprecated. Use the info inside the corresponding ActivityTaskStartedEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, ActivityTaskStartedEvent has no version stamp now
@@ -424,8 +448,11 @@ message ActivityTaskCanceledEventAttributes { | |||
// id of the worker who canceled this activity | |||
string identity = 5; | |||
// Version info of the worker who processed this workflow task. | |||
// Deprecated. Use the info inside the corresponding ActivityTaskStartedEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, ActivityTaskStartedEvent has no version stamp now
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?
Why?
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-zeroramping_version_percentage
is the way to ramp traffic to unversioned workers.The SDK interface for this could look like:
Server PR