Add Create and Update for Worker Deployment Version, update scaler config and enable per-task-type compute providers#731
Conversation
4b74a6b to
448fcaa
Compare
86892e0 to
7ca9f8b
Compare
42cbcff to
7ce67c6
Compare
|
Semgrep found 1 No explicit |
7ce67c6 to
041a11a
Compare
041a11a to
49d52e9
Compare
11be549 to
ae4546c
Compare
ae4546c to
309b565
Compare
Adding a Create API for Worker Deployment Versions, as well as a compute config setting at the version level as that is the matching one. A Deployment Version references a specific running version of a worker, which is also what a compute provider tracks.
Both scaler and compute providers are pluggable, so aligning the formats for easier understanding.
Advanced customers might want to define different compute providers for different task types, so enabling splitting that up.
a3cbeea to
387d6ad
Compare
| // Optional. Contains the compute config scaling groups to remove from the Worker Deployment. | ||
| repeated string remove_compute_config_scaling_groups = 7; |
There was a problem hiding this comment.
I'm not a huge fan of separating out a "remove" list from an "add/update" list, but if this is a pattern used elsewhere in the API, ok :) I prefer a Replace-style pattern where the user passes the exact desired state, including any existing list or map fields and the server simply replaces those fields with the supplied values. Additionally, having a add or remove separate API call to add/remove individual list items from a list field is a good idea. So, in this case, we'd add AddWorkerDeploymentVersionScalingGroup and RemoveWorkerDeploymentVersionScalingGroup API calls...
But again, if this is not a pattern used in the existing API, so be it :)
There was a problem hiding this comment.
Yeah, this was changed from replace-style to add/update based on feedback what is used elsewhere in the API
| string namespace = 1; | ||
|
|
||
| // Required. | ||
| string deployment_name = 2; |
There was a problem hiding this comment.
How is the build ID/version of the WDV identified in this request message?
There was a problem hiding this comment.
There is no build ID/version yet when running this validate API as it is not used in validation
There was a problem hiding this comment.
Hmm. Seems odd to have WorkerDeploymentVersion in the API name, then... Maybe better to go with ValidateComputeConfig?
There was a problem hiding this comment.
Yeah, I had been unhappy with that contradiction as well. That said ValidateComputeConfig seems too generic as well which is why I didn't go with it.
I now reframed it a bit: added in the build ID as well. This way the story is more that the validate-compute-config should mirror the update-compute-config more than the create version API. That means it clearly should have the build ID. Down the road this might also be useful once we can validate that the compute reference points at the configured build ID.
387d6ad to
a19c407
Compare
| // will be an encrypted, encoded bytestring containing | ||
| // provider-specific information. The implementation must understand | ||
| // how to decrypt the payload. | ||
| // Encrypted, encoded bytestring containing provider-specific |
There was a problem hiding this comment.
This would be optionally encrypted.
|
|
||
| // Optional. Contains the compute config scaling groups to add or updated for the Worker | ||
| // Deployment. | ||
| map<string, temporal.api.compute.v1.ComputeConfig.ScalingGroup> compute_config_scaling_groups = 6; |
There was a problem hiding this comment.
I would recommend using field masks for update APIs. We've done that in a few places (see UpdateActivityOptions). This way if you add fields that the client is not aware of they will not accidentally be deleted on update.
There was a problem hiding this comment.
@ShahabT ?
This was what other APIs around here already used.
a19c407 to
5aa7da9
Compare
9c10378 to
a25db8a
Compare
| repeated string remove_compute_config_scaling_groups = 7; | ||
|
|
||
| // Optional. Controls which fields from `compute_config_scaling_groups` will be applied | ||
| google.protobuf.FieldMask update_mask = 8; |
There was a problem hiding this comment.
It would be a separate field per ComputeConfigScalingGroup though, no?
There was a problem hiding this comment.
the field mask could express the map, no? I could make it per entry but that seems somewhat redundant with what the field mask can already do (if I just make it a map<string, FieldMask>).
There was a problem hiding this comment.
I would do either map<string, FieldMask> here or map<string, ComputeConfigScalingGroupUpdate> where ComputeConfigScalingGroupUpdate is a message that has both a ComputeConfigScalingGroup and a FieldMask. I prefer the latter.
| // This value is returned so that it can be optionally passed to APIs | ||
| // that write to the Worker Deployment state to ensure that the state | ||
| // did not change between this API call and a future write. | ||
| bytes conflict_token = 5; |
There was a problem hiding this comment.
I'm also wondering whether the conflict token is even needed here since there's two types of operations:
- Update specific fields
- Remove a group
Both are idempotent on their own and do not rely on a previous state.
There was a problem hiding this comment.
Unless I'm misunderstanding how you expect the updates to be submitted. Partial updates (patches) would be easier to use for users typically and save the need to handle conflicts.
There was a problem hiding this comment.
I think conflicts can still occur, just the scope is more specific so I would argue the pattern of conflict tokens would hold even with partial updates (though would agree they are less critical)
There was a problem hiding this comment.
AFAIC, conflict token can be optional for sensitive updates where there's a risk but just deleting a group or updating a field on a group doesn't require it IMHO.
a25db8a to
4d33b58
Compare
bergundy
left a comment
There was a problem hiding this comment.
LGTM, but I would double check that the implementation that backs this works before merging into main.
4d33b58 to
06629f5
Compare
What changed?