Versioning 3.1 merge #7233
Open
Versioning 3.1 merge #7233
+11,416
−4,419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShahabT
force-pushed
the
versioning-3.1-merge
branch
4 times, most recently
from
001bfbe to
49a0c78
Compare
ShahabT
commented
Feb 4, 2025
ShahabT
commented
Feb 4, 2025
ShahabT
commented
Feb 4, 2025
| // - It has no active pollers (see WorkerDeploymentVersionInfo.pollers_status) | ||
| // - It is drained (see WorkerDeploymentVersionInfo.drainage_status) | ||
| // TODO (Shivam) - Removal of registered versions based on the above conditions. | ||
| repeated string versions = 3; |
There was a problem hiding this comment.
Not urgent, but this can be a map from string to VersionSummary so we don't have to query all version wfs to populate version summaries when DescribeWorkerDeployment is called.
proto/internal/temporal/server/api/persistence/v1/executions.proto
Outdated
Show resolved
Hide resolved
ShahabT
force-pushed
the
versioning-3.1-merge
branch
2 times, most recently
from
83b651c to
214cd5e
Compare
## What changed? <!-- Describe what has changed in this PR --> - Updated deployment entity wf's API's (naming) for versioning-3.1. Removed those which will not be required. - Added functionality for RegisterTaskQueue + DescribeVersion - Added functional tests for verifying the above two work. - UserData stuff has been removed right now since there are pending discussions ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Functional tests have been added. Note, the tests added are exact replicas (in terms of their core functionality) with the existing tests under `deployment_test.go` ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature branch. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> Using new Deployment Options fields sent by SDK in Versioning 3 functionality. Old fields are still used when new fields are absent. Implementation did not change, both new and old fields sent in polls and task responses are still converted to old `Deployment` object and used as before. Later, code will be refactored to change the `Deployment` usages to `DeploymentVersion`. Also added new fields to replace `Deployment` with `DeploymentVersion` fields in internal protos where needed. Matching<->History communication happens via these new fields, only new internal fields are written but both new and old fields are read. ## Why? <!-- Tell your future self why have you made these changes --> Incorporating latest renames in Versioning APIs. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Existing tests changed to use new fields (or both old and new depending on the test). ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> None. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> None yet. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No.
## What changed? <!-- Describe what has changed in this PR --> - DescribeWorkerDeployment API for versioning-3.1 For this API to work, I realized that the worker deployment workflow has to have information about the various deployment versions it encapsulates. Now that version deployment workflows won't have a `deploymentName` present in their workflowID (for now), partial matching workflow ID's with the received deployment name to target version workflows was not feasible. Another option was to store the mapping from "versions" to "the local state of a version workflow". However, this would have made the deployment workflow arguments large in size since in theory, we could have many deployment versions each with large sized local states. The option I ended up going with was storing the version in a deployment workflow during task-queue registration. During a `DescribeWorkerDeployment` call, we would call `Describe` on each stored version and return the aggregated info to the user. A version will be removed from a worker deployment when it becomes "scavenged" (to be implemented) Potential future improvements include adding a caching mechanism. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added a functional test + more tests to follow ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going inside a feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> Matching to Worker Deployment Ramping Version and its percentage when routing tasks. User-facing APIs are not implemented yet. The implementation also supports ramping from a Deployment Version to unversioned workers. ## Why? <!-- Tell your future self why have you made these changes --> Part of Worker Versioning features. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added unit and functional tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> None. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> None. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No --------- Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io>
## What changed? <!-- Describe what has changed in this PR --> - Fix flaky `DescribeWorkerDeployment`. The flake was caused because I forgot to pass in the right parameters when creating version entity workflows. Moreover, recent proto changes meant that my assertions also had to be updated and have done so :) - Other breaking unit tests in the versioning-3.1 repo ## Why? <!-- Tell your future self why have you made these changes --> - don't like seeing red markers after CI is done running ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Ran unit and functional tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going to a feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - title ## Why? <!-- Tell your future self why have you made these changes --> - versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - existing suite of tests + added functional tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature branch ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> - ListWorkerDeployments API - Also removed presence of locks when adding a version to a worker-deployment workflow. Causes deadlocks and we don't need them. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added functional tests (happy-path) - Existing suite ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? Add DrainageStatus child workflow to worker deployment system ## Why? To periodically update the version workflow with results from visibility. ## How did you test it? Functional tests. Currently broken (see todo comment in code) ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io>
…, fix comments (#7178) ## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - Ramping PR for versioning-3.1 - now, if a ramping version becomes current, the ramp will be unset. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added more functional tests - Existing suite ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going to feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No
## What changed? <!-- Describe what has changed in this PR --> - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <57200924+Shivs11@users.noreply.github.com>
## What changed? <!-- Describe what has changed in this PR --> 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? <!-- Tell your future self why have you made these changes --> See temporalio/api#547. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <57200924+Shivs11@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io> Co-authored-by: ShahabT <shahab.tajik@temporal.io>
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - added poller presence (called MissingTaskQueue in code) checks when a version starts to ramp or wants to be set as current - happy path tests for delete version have been added - happy path tests for poller presence checks have been added ## Why? <!-- Tell your future self why have you made these changes --> - versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - NO TESTS have been added - just an initial prototype for review. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
… wf (#7240) ## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
…kQueueFamilyData (#7234) ## What changed? 1. Pass Task Queue Types to `CheckIfTaskQueuesHavePollers` activity so that we check pollers for only the task queue types that are registered in that version. 2. Remove `DeploymentVersionData` from the per-type map in `TaskQueueFamilyData`. `DeploymentVersionData` stores the same info as `VersionLocalState`, so repeating it num_task_queue times inside the VersionLocalState was a waste of space, and kind of confusing. I spoke with Shahab yesterday about how `TaskQueueFamilyData` should contain information that is specific to each task queue type within that version. So far, the only data that is specific to the version + task queue tuple is `first_poller_time`, so I put that in a new `TaskQueueVersionData` struct and put it in `TaskQueueFamilyData`. ## Why? See explanation above. ## How did you test it? Tested that the existing Versioning 3.1 functional tests still work, including DeleteVersion with poller presence. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
… wf (#7240) ## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
ShahabT
force-pushed
the
versioning-3.1-merge
branch
from
374d486 to
8213b09
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Versioning 3.1 APIs:
Why?
More functionality and improving some existing ones based on user feedback.
How did you test it?
Added unit tests and functional tests, but testing is not done for the new APIs, more to come.
Potential risks
None identified.
Documentation
None yet.
Is hotfix candidate?
No.