Use new versioning fields #7119
Conversation
…ctive-3.1 # Conflicts: # common/testing/testvars/test_vars.go # go.mod # go.sum # service/matching/matching_engine.go
| @@ -100,7 +100,7 @@ func NewResult( | |||
|
|
|||
| func (s *workerComponent) DedicatedWorkerOptions(ns *namespace.Namespace) *workercommon.PerNSDedicatedWorkerOptions { | |||
There was a problem hiding this comment.
I just opened a PR to fix this actually: #7123
No worries though, the changes seem to be the same
| Version: tv.BuildID(), | ||
| Name: tv.DeploymentSeries(), |
There was a problem hiding this comment.
we can now use: tv.DeploymentName() and tv.DeploymentVersion
There was a problem hiding this comment.
Hmm... I'm actually removing them in https://github.com/temporalio/temporal/pull/7126/files#diff-969e474fa5d1098f4d0735292efd98defd6af7cc7cf4506613dc5e07991f4c46R263. The idea is we just use old BuildID and DeploymentSeries testvars in all places. Until we refactor the code and get rid of old names altogether. The problem with having two testvars (say DeploymentSeries and DeploymentName) is that test helper functions don't know to use which one and that defeats the purpose of testvars.
There was a problem hiding this comment.
test helper functions don't know to use which one and that defeats the purpose of testvars
can we not just write newer tests with the updated test-vars names and then when removing the old versioning names altogether, remove the old test-vars objects too?
There was a problem hiding this comment.
We can do that, but that would require creating a clone of versioning_3_tests.go (which is not a bad idea, but not what I did, I just repurposed the tests to use new proto fields.)
| if ms.GetExecutionInfo().GetVersioningInfo() == nil { | ||
| ms.GetExecutionInfo().VersioningInfo = &workflowpb.WorkflowExecutionVersioningInfo{} | ||
| if override != nil { | ||
| if ms.GetExecutionInfo().GetVersioningInfo() == nil { |
There was a problem hiding this comment.
do we wanna set ms.GetExecutionInfo().VersioningInfo = &workflowpb.WorkflowExecutionVersioningInfo{} regardless of override being nil or not?
There was a problem hiding this comment.
Good point, the idea was if override is nil it means we have an override already and want to unset it, hence ms.GetExecutionInfo().VersioningInfo has already a value. But I don't think it's wise to make that assumption. I improved the else block in line 4423.
There was a problem hiding this comment.
nice, this looks much better - thanks for addressing
What changed?
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
Deploymentobject and used as before. Later, code will be refactored to change theDeploymentusages toDeploymentVersion.Also added new fields to replace
DeploymentwithDeploymentVersionfields 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?
Incorporating latest renames in Versioning APIs.
How did you test it?
Existing tests changed to use new fields (or both old and new depending on the test).
Potential risks
None.
Documentation
None yet.
Is hotfix candidate?
No.