ListWorkerDeployments : Versioning-3.1#7173
Merged
Shivs11 merged 8 commits intoversioning-3.1from Jan 28, 2025
Merged
Conversation
Shivs11
commented
Jan 28, 2025
Comment on lines
-661
to
-672
| // userData, _, err := c.partitionMgr.GetUserDataManager().GetUserData() | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
|
|
||
| // deploymentVersionData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentVersionData() | ||
| // if findDeploymentVersion(deploymentVersionData, workerDeployment.SeriesName, workerDeployment.BuildId) >= 0 { | ||
| // // already registered in user data, we can assume the workflow is running. | ||
| // // TODO: consider replication scenarios where user data is replicated before | ||
| // // the deployment workflow. | ||
| // return nil | ||
| // } |
Member
Author
There was a problem hiding this comment.
had forgotten to uncomment this and the next block of code - important since it basically serves as a de-dup mechanism for multiple polls coming in with the same worker-deployment information!
carlydf
reviewed
Jan 28, 2025
| request.PageSize = maxPageSize | ||
| } | ||
|
|
||
| resp, nextPageToken, err := wh.workerDeploymentClient.ListWorkerDeployments(ctx, namespaceEntry, int(request.PageSize), request.NextPageToken) |
Contributor
There was a problem hiding this comment.
I don't think it's worth changing for all of these APIs, but personally I find it easier to write code / keep up with shifting parameters if the input to my helper function (in this case workerDeploymentClient.ListWorkerDeployments) is just the proto request object + namespaceEntry. Then all the necessary parameters are already present.
Up to you though and I think not worth changing now, just letting you know for the future that it's totally fine to pass the whole request object if that's easier for you.
Member
Author
There was a problem hiding this comment.
I think this was a design decision which was followed since the pre-release days and I didn't wanna alter it - fwiw, I agree with you - I shall keep things as is for now but not repeat the same thing in future
Comment on lines
-661
to
-672
| // userData, _, err := c.partitionMgr.GetUserDataManager().GetUserData() | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
|
|
||
| // deploymentVersionData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentVersionData() | ||
| // if findDeploymentVersion(deploymentVersionData, workerDeployment.SeriesName, workerDeployment.BuildId) >= 0 { | ||
| // // already registered in user data, we can assume the workflow is running. | ||
| // // TODO: consider replication scenarios where user data is replicated before | ||
| // // the deployment workflow. | ||
| // return nil | ||
| // } |
Comment on lines
+698
to
+700
| deploymentVersions := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData() | ||
| if findDeploymentVersion(deploymentVersions, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 { | ||
| break |
Contributor
There was a problem hiding this comment.
Suggested change
| deploymentVersions := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData() | |
| if findDeploymentVersion(deploymentVersions, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 { | |
| break | |
| deploymentData := userData.GetData().GetPerType()[int32(c.queue.TaskType())].GetDeploymentData() | |
| if findDeploymentVersion(deploymentData, worker_versioning.DeploymentVersionFromDeployment(workerDeployment)) >= 0 { | |
| break |
This is still deploymentData, it just has a Deployments field and a Versions field now.
| select { | ||
| case <-userDataChanged: | ||
| case <-ctx.Done(): | ||
| c.logger.Error("timed out waiting for deployment to appear in user data") |
Contributor
There was a problem hiding this comment.
Suggested change
| c.logger.Error("timed out waiting for deployment to appear in user data") | |
| c.logger.Error("timed out waiting for worker deployment version to appear in user data") |
Is this more accurate?
| ctx, | ||
| namespaceEntry, | ||
| WorkerDeploymentVersionWorkflowType, | ||
| WorkerDeploymentWorkflowType, |
Contributor
There was a problem hiding this comment.
nice catch -- although I'm confused why our tests didn't catch this. Seems like sending an update to the wrong WF type should have always failed
Member
Author
There was a problem hiding this comment.
I know - I wondered on this too....
|
|
||
| // Namespace division | ||
| WorkerDeploymentVersionNamespaceDivision = "TemporalWorkerDeploymentVersion" | ||
| WorkerDeploymentVersionNamespaceDivision = "TemporalWorkerDeployment" |
Contributor
There was a problem hiding this comment.
Should this variable be called WorkerDeploymentNamespaceDivision ?
Contributor
There was a problem hiding this comment.
I think the naming with "version" is left over from when "Deployment" (instance) was the "first-class-citizen" of this whole API
Member
Author
There was a problem hiding this comment.
oops, yeah you are right - changing this to be called WorkerDeploymentNamespaceDivision
Comment on lines
-241
to
-251
| // add version to worker-deployment workflow | ||
| activityCtx = workflow.WithActivityOptions(ctx, defaultActivityOptions) | ||
| err = workflow.ExecuteActivity(activityCtx, d.a.AddVersionToWorkerDeployment, &deploymentspb.AddVersionToWorkerDeploymentRequest{ | ||
| DeploymentName: d.VersionState.DeploymentName, | ||
| Version: d.VersionState.Version, | ||
| RequestId: d.newUUID(ctx), | ||
| }).Get(ctx, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
Member
Author
There was a problem hiding this comment.
ah sorry - we were adding the same version twice to the worker-deployment workflow (we were doing this twice)!
Contributor
There was a problem hiding this comment.
oh I see the duplicate code above -- got it
|
|
||
| func (d *WorkflowRunner) validateSetCurrent(args *deploymentspb.SetCurrentVersionArgs) error { | ||
| if d.State.CurrentVersion != args.Version { | ||
| if d.State.RoutingInfo.CurrentVersion != args.Version { |
Contributor
There was a problem hiding this comment.
Nit: I think it's more idiomatic in Go, and more readable, to structure the function like
func foo(...) error {
if errorCondition {
return err
}
return nil
}
Returning success at the end and errors in the middle makes it easier to read. The section "A longer example" in https://8thlight.com/insights/exploring-error-handling-patterns-in-go this article also calls this out with another example.
carlydf
approved these changes
Jan 28, 2025
ShahabT
pushed a commit
that referenced
this pull request
Feb 4, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 5, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 5, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
ShahabT
pushed a commit
that referenced
this pull request
Feb 6, 2025
## 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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?