Merged
Conversation
…ctive-3.1 # Conflicts: # common/testing/testvars/test_vars.go # go.mod # go.sum # service/matching/matching_engine.go
Shivs11
reviewed
Jan 21, 2025
Member
Shivs11
left a comment
Shivs11
left a comment
There was a problem hiding this comment.
just a few comments since i'm unsure about one/two things - for the most part though, this is good
| @@ -100,7 +100,7 @@ func NewResult( | |||
|
|
|||
| func (s *workerComponent) DedicatedWorkerOptions(ns *namespace.Namespace) *workercommon.PerNSDedicatedWorkerOptions { | |||
Member
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
Comment on lines
+428
to
+429
| Version: tv.BuildID(), | ||
| Name: tv.DeploymentSeries(), |
Member
There was a problem hiding this comment.
we can now use: tv.DeploymentName() and tv.DeploymentVersion
Contributor
Author
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.
Member
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?
Contributor
Author
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 { |
Member
There was a problem hiding this comment.
do we wanna set ms.GetExecutionInfo().VersioningInfo = &workflowpb.WorkflowExecutionVersioningInfo{} regardless of override being nil or not?
Contributor
Author
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.
Member
There was a problem hiding this comment.
nice, this looks much better - thanks for addressing
Shivs11
approved these changes
Jan 21, 2025
ShahabT
added a commit
that referenced
this pull request
Feb 4, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 5, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 5, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## 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.
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## 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.
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?
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.