Add TemporalUsedWorkerDeploymentVersions SA #8886
Conversation
carlydf
left a comment
carlydf
left a comment
There was a problem hiding this comment.
I didn't review the tests yet, but this looks good so far! I really appreciate you taking it on.
| TemporalWorkerDeployment: enumspb.INDEXED_VALUE_TYPE_KEYWORD, | ||
| TemporalChangeVersion: enumspb.INDEXED_VALUE_TYPE_KEYWORD_LIST, | ||
| BinaryChecksums: enumspb.INDEXED_VALUE_TYPE_KEYWORD_LIST, | ||
| BuildIds: enumspb.INDEXED_VALUE_TYPE_KEYWORD_LIST, |
There was a problem hiding this comment.
Do we want to comment BuildIds as deprecated at this time, or are you saving that for later?
There was a problem hiding this comment.
good point. will do. for all intents and purposes BuildIds will be considered deprecated after this change.
| limit := ms.config.SearchAttributesSizeOfValueLimit(string(ms.namespaceEntry.Name())) | ||
| if _, err := ms.addBuildIdAndDeploymentInfoToSearchAttributesWithNoVisibilityTask(event.SourceVersionStamp, limit); err != nil { | ||
| // Passing nil for usedVersion because starting with pinned override does not add the version to used versions SA until the version is actally used. | ||
| if _, err := ms.addBuildIdAndDeploymentInfoToSearchAttributesWithNoVisibilityTask(event.SourceVersionStamp, nil, limit); err != nil { |
There was a problem hiding this comment.
ok, so we update the SAs for version, behavior, and deployment name on task started (and on successful set-override), but only update used-versions on task completed?
makes sense
| // This is also useful for unversioned workers. | ||
| // For v1 and v2 versioned workflows the search attributes should be already up-to-date based on the task started events. | ||
| if err := m.ms.updateBuildIdsAndDeploymentSearchAttributes(attrs.GetWorkerVersion(), limits.MaxSearchAttributeValueSize); err != nil { | ||
| if err := m.ms.updateBuildIdsAndDeploymentSearchAttributes(attrs.GetWorkerVersion(), worker_versioning.ExternalWorkerDeploymentVersionFromDeployment(wftDeployment), limits.MaxSearchAttributeValueSize); err != nil { |
There was a problem hiding this comment.
I wish we could just pass attrs.GetDeploymentVersion() instead of wftDeployment here, but I suppose wftDeployment is correct for older SDKs that are sending deprecated attrs.GetDeployment() or deprecated attrs.GetWorkerDeploymentVersion() values?
As a follow-up: Should we be passing a form of wftDeployment as the first parameter instead of attrs.GetWorkerVersion()?
There was a problem hiding this comment.
yeah I thought about that too. but I decided against using deprecated types for new args. but I think we're close to getting rid of all the v0.3.0-0.3.1 types. in next Go SDK and CLI we should completely get rid of them (by GA) and then we can remove them from server.
# Conflicts: # tests/versioning_3_test.go
| if event.GetInheritedAutoUpgradeInfo() != nil { | ||
| ms.SetVersioningRevisionNumber(event.GetInheritedAutoUpgradeInfo().GetSourceDeploymentRevisionNumber()) | ||
| // TODO (Shivam): Remove this once you make SetDeploymentVersion and SetVersioningBehavior methods with nil checks | ||
| if ms.executionInfo.VersioningInfo == nil { | ||
| ms.executionInfo.VersioningInfo = &workflowpb.WorkflowExecutionVersioningInfo{} | ||
| } | ||
| ms.executionInfo.VersioningInfo.DeploymentVersion = event.GetInheritedAutoUpgradeInfo().GetSourceDeploymentVersion() | ||
| // Assume AutoUpgrade behavior for the first workflow task. | ||
| ms.executionInfo.VersioningInfo.Behavior = enumspb.VERSIONING_BEHAVIOR_AUTO_UPGRADE |
There was a problem hiding this comment.
i'm slightly confused - any specific reason of moving this to the top compared to where it was before?
| // Sets TemporalWorkerDeployment to the override DeploymentName if present, or`ms.executionInfo.WorkerDeploymentName`. | ||
| // Sets TemporalWorkerDeploymentVersion to the override PinnedVersion if present, or `ms.executionInfo.VersioningInfo.Version`. | ||
| // Sets TemporalWorkflowVersioningBehavior to the override Behavior if present, or `ms.executionInfo.VersioningInfo.Behavior` if specified. |
There was a problem hiding this comment.
nit: could update comment here because we are now adding a new SA
| if deployment == "" { | ||
| saPayloads[sadefs.TemporalWorkerDeployment] = nil | ||
| } else { | ||
| deploymentPayload, err := searchattribute.EncodeValue(deployment, enumspb.INDEXED_VALUE_TYPE_KEYWORD) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(deploymentPayload.GetData()) <= maxSearchAttributeValueSize { // we know the string won't really be over, but still check | ||
| saPayloads[sadefs.TemporalWorkerDeployment] = deploymentPayload | ||
| } |
There was a problem hiding this comment.
nit: a lot of the code here seems to be following the pattern of setting the SA value to be nil if a passed arg is nil. If it's not, you encode it with the INDEXED_VALUE_TYPE_KEYWORD.
Could def make a helper function here for clarity
Shivs11
left a comment
Shivs11
left a comment
There was a problem hiding this comment.
some small nits and one question - not blocking tho
What changed?
Add
TemporalUsedWorkerDeploymentVersionssearch attribute to contain all the Deployment Versions that completed at least one workflow tasks for the execution.This Search Attribute replaces, now deprecated,
BuildIdsSearch attribute.Why?
Needed a replacement for
BuildIdsSearch attribute which holds internal values and prefixes.How did you test it?
Potential risks
None.