Describe worker deployment in versioning 3.1 #7127
Merged
Conversation
This file contains hidden or 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
carlydf
reviewed
Jan 22, 2025
Comment on lines
82
to
83
| // experimental | ||
| repeated string versions = 3; |
There was a problem hiding this comment.
The other fields are pretty easy to understand what they are for, but the versions field is not immediately clear. Could you please comment here explaining what this is, what the order of the list means or doesn't mean, etc?
| @@ -683,7 +791,7 @@ func (d *ClientImpl) record(operation string, retErr *error, args ...any) func() | |||
| } | |||
|
|
|||
| //nolint:staticcheck | |||
| func stateToInfo(state *deploymentspb.VersionLocalState) *deploymentpb.WorkerDeploymentVersionInfo { | |||
| func VersionStateToVersionInfo(state *deploymentspb.VersionLocalState) *deploymentpb.WorkerDeploymentVersionInfo { | |||
There was a problem hiding this comment.
I think it's better to not export helper functions if they aren't used outside of the package. (Ignore this comment if this is used outside of the package and I missed where)
| defer d.record("DescribeWorkerDeployment", &retErr, deploymentName)() | ||
|
|
||
| // validating params | ||
| err := ValidateVersionWfParams(WorkerDeploymentFieldName, deploymentName, d.maxIDLengthLimit()) |
There was a problem hiding this comment.
can this be lowercase or do you call it in another package too?
There was a problem hiding this comment.
good point about the exports - shall be more wary of this going forward.
|
|
||
| success := outcome.GetSuccess() | ||
| if success == nil { | ||
| return nil, serviceerror.NewInternal("outcome missing success and failure") |
There was a problem hiding this comment.
I think we should include the version that failed to be added in the error message
| @@ -708,3 +816,28 @@ func stateToInfo(state *deploymentspb.VersionLocalState) *deploymentpb.WorkerDep | |||
| TaskQueueInfos: taskQueues, | |||
| } | |||
| } | |||
|
|
|||
| func (d *ClientImpl) DeploymentStateToDeploymentInfo(ctx context.Context, namespaceEntry *namespace.Namespace, | |||
There was a problem hiding this comment.
same question here: does this need to be an exported function?
| @@ -102,6 +108,16 @@ type Client interface { | |||
| identity string, | |||
| requestID string, | |||
| ) (*deploymentspb.SyncVersionStateResponse, error) | |||
|
|
|||
| // Only used internally by Worker Deployment Version workflow: | |||
There was a problem hiding this comment.
Suggested change
| // Only used internally by Worker Deployment Version workflow: | |
| // Used internally by the Worker Deployment Version workflow in its AddVersionToWorkerDeployment Activity |
| @@ -218,6 +218,17 @@ func (d *VersionWorkflowRunner) handleRegisterWorker(ctx workflow.Context, args | |||
| // } | |||
There was a problem hiding this comment.
what is your plan for syncing to user data of the task queues as in the commented-out section? is that coming in a later PR?
There was a problem hiding this comment.
Will be coming in the very next PR, which depends on this PR for getting in
|
|
||
| for _, v := range d.State.Versions { | ||
| if v == version { | ||
| return temporal.NewApplicationError("deplopyment version already registered", errVersionAlreadyExistsType) |
There was a problem hiding this comment.
Suggested change
| return temporal.NewApplicationError("deplopyment version already registered", errVersionAlreadyExistsType) | |
| return temporal.NewApplicationError("deployment version already registered", errVersionAlreadyExistsType) |
tests/worker_deployment_test.go
Outdated
Comment on lines
137
to
138
| // Name is used by testvars. We use a shorten test name in variables so that physical task queue IDs | ||
| // do not grow larger that DB column limit (currently as low as 272 chars). |
There was a problem hiding this comment.
Suggested change
| // Name is used by testvars. We use a shorten test name in variables so that physical task queue IDs | |
| // do not grow larger that DB column limit (currently as low as 272 chars). | |
| // Name is used by testvars. We use a shortened test name in variables so that physical task queue IDs | |
| // do not grow larger than DB column limit (currently as low as 272 chars). |
carlydf
reviewed
Jan 22, 2025
| @@ -79,6 +79,9 @@ message WorkerDeploymentWorkflowArgs { | |||
| message WorkerDeploymentLocalState { | |||
| string current_version = 1; | |||
| google.protobuf.Timestamp current_changed_time = 2; | |||
| // Versions that have been registered in this deployment. | |||
There was a problem hiding this comment.
Sorry to be so nit-picky about this comment, but I was curious about whether items will ever be removed from this list, so I consulted the notion doc which says this:
// Versions that are currently tracked in this Worker Deployment. A Version will be
// cleaned up automatically if all the following conditions meet:
// - It does not receive new executions (see WorkerDeploymentVersionInfo.accepts_new_executions)
// - It has no active pollers (see WorkerDeploymentVersionInfo.pollers_status)
// - It is drained (see WorkerDeploymentVersionInfo.drainage_status)
If the goal is to eventually add this cleaning up logic, but it's not implemented yet, perhaps a TODO comment saying that we eventually intend to clean up versions from this list on the above conditions
There was a problem hiding this comment.
yes, good point - sorry, you shouldn't have had to consulting the notion document since thats what comments are meant to be for - moreover, I like the comment you have pasted and will use it in this message for better clarity.
carlydf
approved these changes
Jan 22, 2025
ShahabT
pushed a commit
that referenced
this pull request
Feb 4, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 4, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 5, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 5, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
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?
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
deploymentNamepresent 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
DescribeWorkerDeploymentcall, we would callDescribeon 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?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?