Worker versioning related updates #272
Conversation
| // All the compatible versions, in insertion order. | ||
| // The last element is considered the set "default" and may not be the last inserted item in case of promoting an | ||
| // older build ID to be the set default. | ||
| repeated string build_ids = 1; |
There was a problem hiding this comment.
Is it safe to change the id from 2 to 1?
There was a problem hiding this comment.
This API isn't in use yet
There was a problem hiding this comment.
Not in general, but this is not in use anywhere yet
| // All the compatible versions, in insertion order. | ||
| // The last element is considered the set "default" and may not be the last inserted item in case of promoting an | ||
| // older build ID to be the set default. | ||
| repeated string build_ids = 1; |
There was a problem hiding this comment.
Not in general, but this is not in use anywhere yet
| // All the compatible versions, in insertion order. | ||
| // The last element is considered the set "default" and may not be the last inserted item in case of promoting an | ||
| // older build ID to be the set default. |
There was a problem hiding this comment.
this is contradictory.. it can't be both "in insertion order" and "the last element ... may not be the last inserted item"
There was a problem hiding this comment.
I think he means "need not" rather than "may not" - but yeah it's still confusing, this needs to distinguish insertion time from insertion order
There was a problem hiding this comment.
This is what we decided though..
If we have keep the timestamps internally, we can preserve the insertion order but we also wanted the default to be the last.
There was a problem hiding this comment.
What are you suggesting?
There was a problem hiding this comment.
Maybe just remove the first line? They're in arbitrary order except that the last one is special
There was a problem hiding this comment.
It's nice that we can preserve the insertion order though, why take that away?
I'm almost tempted to add timestamps (wall clock) to each build ID to give users some extra bit of useful information.
Another nice bit of information would be identity and reason for each build ID but I'm hesitant to open this can of worms.
temporal/api/common/v1/message.proto
Outdated
|
|
||
| // If set, the worker is opting in to worker versioning. Otherwise, the version stamp is used as a marker for | ||
| // workflow reset points and the BuildId search attibute. | ||
| bool uses_versioning = 3; |
There was a problem hiding this comment.
hmm, I would think this flag should not be part of the stamp itself, but part of the requests that include a stamp
|
Changed the base branch to |
|
Comments still welcome of course. |
bergundy
changed the title
Decided not to expose (the yet unused) `CompatibleVersionSet.version_set_id`, the set will have multiple IDs internally. The affected projects have been updated: - [x] [Server PR](temporalio/temporal#4170) - [x] [Go SDK PR](temporalio/sdk-go#1089) Also added a `use_versioning` flag to `RespondWorkflowTaskCompletedRequest` to differentiate between the version stamp being used for versioning (matching) purposes or just as a marker.
Decided not to expose (the yet unused) `CompatibleVersionSet.version_set_id`, the set will have multiple IDs internally. The affected projects have been updated: - [x] [Server PR](temporalio/temporal#4170) - [x] [Go SDK PR](temporalio/sdk-go#1089) Also added a `use_versioning` flag to `RespondWorkflowTaskCompletedRequest` to differentiate between the version stamp being used for versioning (matching) purposes or just as a marker.
Decided not to expose (the yet unused)
CompatibleVersionSet.version_set_id, the set will have multiple IDs internally.The affected projects have been updated:
Also added a
use_versioningflag toRespondWorkflowTaskCompletedRequestto differentiate between the version stamp being used for versioning (matching) purposes or just as a marker.