Implement UpdateWorkerDeploymentVersionComputeConfig#9746
Implement UpdateWorkerDeploymentVersionComputeConfig#9746ShahabT merged 3 commits intoserverlessfrom
Conversation
| for name, sg := range scalingGroups { | ||
| specs[name] = wciiface.ScalingGroupSpec{ | ||
| TaskTypes: sg.GetTaskQueueTypes(), | ||
| Compute: wciiface.ComputeProviderSpec{ |
There was a problem hiding this comment.
also set the scaling spec?
| } | ||
|
|
||
| // Apply the validated config to state. | ||
| d.VersionState.ComputeConfig = newConfig |
There was a problem hiding this comment.
you will need to call the Worker Controller Instance Client Update method to actually apply the config
| } | ||
|
|
||
| // used as Worker Deployment Version workflow update input: | ||
| message UpdateVersionComputeConfigArgs { |
There was a problem hiding this comment.
nit: can we drop this version here?
| map<string, temporal.api.compute.v1.ComputeConfigScalingGroup> scaling_groups = 1; | ||
| } | ||
|
|
||
| // used as Worker Deployment Version workflow update input: |
There was a problem hiding this comment.
nit: Input for the UpdateComputeConfig workflow update on a version workflow.
| temporal.api.deployment.v1.WorkerDeploymentInfo.WorkerDeploymentVersionSummary ramping_version_summary = 6; | ||
| } | ||
|
|
||
| // used as activity input for validating compute config scaling groups via |
There was a problem hiding this comment.
nit: Input for the activity that validates ...
| string request_id = 2; | ||
| // Scaling groups to add or update. | ||
| map<string, ScalingGroupUpdate> upsert_scaling_groups = 3; | ||
| // Names of scaling groups to remove. |
There was a problem hiding this comment.
Names that don't match an existing group are ignored.
| // Supports top-level fields and one level of nesting (e.g. "provider.type"). | ||
| func applyFieldMask(dst, src *computepb.ComputeConfigScalingGroup, paths []string) { | ||
| for _, path := range paths { | ||
| switch path { |
There was a problem hiding this comment.
Should we fail if we encounter an unsupported field?
| } | ||
|
|
||
| if err := d.computeConfigLock.Lock(ctx); err != nil { | ||
| d.logger.Error("Could not acquire compute config lock") |
There was a problem hiding this comment.
Should we add some context here like what workflow, etc?
There was a problem hiding this comment.
we're using the sdk logger which has the workflow info
| // also lock compute config lock to ensure that the compute config is not updated while the version is being deleted. | ||
| err = d.computeConfigLock.Lock(ctx) | ||
| if err != nil { | ||
| d.logger.Error("Could not acquire compute config lock") |
There was a problem hiding this comment.
Should we add some context here like what workflow, etc?
|
|
||
| if err := d.computeConfigLock.Lock(ctx); err != nil { | ||
| d.logger.Error("Could not acquire compute config lock") | ||
| return nil, serviceerror.NewDeadlineExceeded("Could not acquire compute config lock") |
There was a problem hiding this comment.
Why not return the original error? Deadline exceeded can be misleading.
| // it's better to CaN because some history events are built now. | ||
| d.setStateChanged() | ||
| d.lock.Unlock() | ||
| d.computeConfigLock.Unlock() |
There was a problem hiding this comment.
nit: Convention is to release the lock in reverse order.
computeConfigLock.Unlock()
lock.Unlock();
| a: nil, | ||
| logger: sdklog.With(workflow.GetLogger(ctx), "wf-namespace", versionWorkflowArgs.NamespaceName), | ||
| metrics: workflow.GetMetricsHandler(ctx).WithTags(map[string]string{"namespace": versionWorkflowArgs.NamespaceName}), | ||
| lock: workflow.NewMutex(ctx), |
There was a problem hiding this comment.
Suggest adding: // Lock ordering: always acquire lock before computeConfigLock.
| groupSpec := wciiface.ScalingGroupSpec{ | ||
| TaskTypes: sg.GetTaskQueueTypes(), | ||
| Compute: wciiface.ComputeProviderSpec{ | ||
| ProviderType: wciiface.ComputeProviderType(sg.GetProvider().GetType()), |
There was a problem hiding this comment.
the FromPayload() conversion stuff in temporal-auto-scaled-workers
this ^
There was a problem hiding this comment.
@ShahabT OK, in that case, we will need to change the client interface and structs in temporal-auto-scaled-workers, yes?
4 participants
What changed?
Implement UpdateWorkerDeploymentVersionComputeConfig
Why?
For server-scaled workers
How did you test it?
Potential risks
None