Versioning 2 API #393

ShahabT · 2024-04-09T18:18:18Z

What changed?

UpdateWorkerVersioningRules API
GetWorkerVersioningRules API
Enhanced DescribeTaskQueue API
Added fields to the following proto messages to track versioning info
- WorkflowExecutionInfo
- PendingActivityInfo
- WorkflowExecutionStartedEventAttributes
- WorkflowTaskStartedEventAttributes
- ActivityTaskStartedEventAttributes
Wire-safe renaming of use_compatible_version to inherit_build_id
Getting rid of bundle_id field in WorkerVersionStamp. (the field is not used in any SDK, nor in server)

Why?
The new API provides better versioning experience for users, and provides a better foundation for us to build more features on top of.

Breaking changes
Removed bundle_id field in WorkerVersionStamp (but it's confirmed not to be used anywhere, so no breaking change really)

Server PR
temporalio/temporal#5691

* Add create_time to rules list output * address comments

* Add versioning info to pending activities and started tasks * address feedback * improved comment * apply suggestions

* clean up versioning protos with the narrowed scope * address comments

Enhanced DescribeTaskQueue

…QueuePartition (#379)

* add redirect_counter filed and more * use int64 * return rules in update response * remove empty line * address comments

# Conflicts: # temporal/api/history/v1/message.proto # temporal/api/workflow/v1/message.proto # temporal/api/workflowservice/v1/request_response.proto

cretz

LGTM. Notes/questions:

Can you confirm that, besides the SDK replaying now-changed history JSON, there are no other issues you can think of in any SDK with the runtime component of workflows? Meaning no workers or workflows will break because of this? (but the administrative side it may make sense to break)
Let's add a "WARNING: Versioning is not yet stable and the API and behavior may change incompatibly" above the unstable pieces. We don't technically have to and we didn't last time and we for sure will mark them this way in the SDK interfacing with these APIs, but I think it's also good here.

cretz · 2024-04-09T20:12:10Z

temporal/api/history/v1/message.proto

It should be noted, this actually will break workflow replays for users using versioning today. Today when people download/save history, they use JSON format, and they keep this JSON format to run on replayers in SDKs. Changing field names is a backwards incompatible history format change from the perspective of replayers.

This may be ok, but it needs to be made very clear. We also may want to offer a way in CLI or something to "fix" invalid history, up to y'all.

Thanks for bringing up the replay history from JSON issue. I did not think about that. Will make sure we mention that in the docs. Maybe some instructions for the user to replace "use_compatible_version" with "inherit_build_id" in the JSON string would suffice.

Regarding the first question, I cannot think of anything else that can break. We have functional tests for old and new versioning (and of course unversioned wfs). The "features" test pass in the versioning-2 branch which use old/current SDKs. As you noticed, the changes are mostly for the admin side, not the data plane. the fields added to data-plain are all optional and mostly for the use of server itself, and only for versioned workflows.

Added the warning to the RPC calls.

Regarding the first question, there is a problem with older UI/CLI (like only recently changed IIRC) where even unset fields are present in the JSON, but most of our replayers ignore unknown JSON fields. Also, sorry I forget, but can you tell whether any of the fields that were changed in the history events are set implicitly in the SDK instead of by explicitly by the workflow author (e.g. as a result of worker setting only)?

I will circulate the history JSON change internally to gather thoughts there.

# Conflicts: # temporal/api/workflow/v1/message.proto

dnr · 2024-04-09T22:13:16Z

temporal/api/taskqueue/v1/message.proto

+message TaskQueueVersionInfo {
+    // Empty means unversioned.
+    string build_id = 1;
+    repeated TaskQueueTypeInfo types_info = 3;


It's not a huge difference, but consider making this a map<temporal.api.enums.v1.TaskQueueType, TaskQueueTypeInfo> and removing type from that? (sorry, I didn't review this on the feature branch)

it seems map key cannot be enum

Huh. It could be int32 with a comment, though? Now it's weird to have one level be a map and the next level not.

dnr · 2024-04-09T22:13:51Z

temporal/api/taskqueue/v1/message.proto

+// For workflow task queues, we only report the normal queue metrics, not sticky queues. This means the stats
+// reported here do not count all workflow tasks. However, because the tasks queued in sticky queues only remain
+// valid for a few seconds, the inaccuracy becomes less significant as the backlog age grows.
+message BacklogInfo {


should we comment this whole thing out for now, if we're not using it?

removed this for now

dnr · 2024-04-09T22:19:21Z

temporal/api/enums/v1/task_queue.proto

+
+enum DescribeTaskQueueMode {
+    // Unspecified means legacy behavior.
+    DESCRIBE_TASK_QUEUE_MODE_UNSPECIFIED = 0;


this means if you don't want to use ENHANCED mode, you have to use UNSPECIFIED, which is kind of weird. should there be an explicit BASIC and then have UNSPECIFIED default to BASIC?

the idea is no one should need to ask for basic mode

not even if they want a cheaper call?

If by cheaper you mean something that only looks at local info for root partition and does not fan out to all partition, I'd still want to put that as an option for the enhanced mode so we return that result in the new structure. (of course if we really thing the cheap option would be useful enough).

Hmm, as long as we can turn off the reachability queries, which I think is the real expensive part, that's okay. Fan-out is fine to have by default (it should always have been doing that).

But what about the "internal" stuff that's deprecated? ack level, rate limit, etc. We don't have InternalTaskQueueStatus yet so now there's no "supported" way to get that info?

The plan is to add a tdbg command for the internal info before CLIs stop supporting legacy mode.

ok, I'm fine then (not a big deal anyway)

dnr · 2024-04-09T22:19:34Z

temporal/api/workflowservice/v1/request_response.proto

    temporal.api.taskqueue.v1.TaskQueueStatus task_queue_status = 2;
+
+    // Only set in `ENHANCED` mode.
+    repeated temporal.api.taskqueue.v1.TaskQueueVersionInfo versions_info = 3;


maybe this should be a map<string, TaskQueueVersionInfo> and remove build_id from that?

ShahabT · 2024-04-10T16:42:34Z

The CI fails for expected reasons due to field renames. Need to an admin to bypass the check.

cretz

Confirmed with SDK team that the field name changes in JSON should be ok

dnr · 2024-04-10T20:38:47Z

temporal/api/enums/v1/task_queue.proto

+
+enum DescribeTaskQueueMode {
+    // Unspecified means legacy behavior.
+    DESCRIBE_TASK_QUEUE_MODE_UNSPECIFIED = 0;


ok, I'm fine then (not a big deal anyway)

ShahabT and others added 15 commits January 11, 2024 19:04

New Worker Versioning API

6c42517

Merge remote-tracking branch 'origin/master' into versioning-2

5d59b6f

Add create_time to rules list output (#346)

ca9d868

* Add create_time to rules list output * address comments

Merge remote-tracking branch 'origin/master' into versioning-2

33adf4c

Add versioning info to pending activities and started tasks (#350)

092b4f4

* Add versioning info to pending activities and started tasks * address feedback * improved comment * apply suggestions

Clean up versioning protos with the narrowed scope (#358)

c05b944

* clean up versioning protos with the narrowed scope * address comments

DescribeTaskQueue improvements (#360)

efc012e

Enhanced DescribeTaskQueue

fix task reachability field name

6d31e31

add inherited_build_id to ExecutionInfo (#373)

01bd740

update comment to describe required conflict token behavior (#375)

7f1078e

rename ListWorkerVersioningRules --> GetWorkerVersioningRules (#376)

f67f905

make TaskQueueVersionGroup message for reuse in internal DescribeTask…

bd24ce7

…QueuePartition (#379)

Add redirect_counter field to task started events and more (#384)

0483b81

* add redirect_counter filed and more * use int64 * return rules in update response * remove empty line * address comments

minor enhancements to versioning API (#390)

93231bd

Merge remote-tracking branch 'origin/master' into versioning-2

8f246fa

# Conflicts: # temporal/api/history/v1/message.proto # temporal/api/workflow/v1/message.proto # temporal/api/workflowservice/v1/request_response.proto

ShahabT requested review from a team as code owners April 9, 2024 18:18

improve reachability exception comment

5096a1f

cretz reviewed Apr 9, 2024

View reviewed changes

ShahabT added 3 commits April 9, 2024 23:17

Merge remote-tracking branch 'origin/master' into versioning-2

2992fb3

# Conflicts: # temporal/api/workflow/v1/message.proto

add stability comment

a8ad66f

fix field id

6e22b14

dnr reviewed Apr 9, 2024

View reviewed changes

address comments

d98f69b

ShahabT mentioned this pull request Apr 10, 2024

Bump api-go to 1.32.0 to include Versioning 2 API changes temporalio/sdk-go#1439

Merged

ShahabT enabled auto-merge (squash) April 10, 2024 16:42

cretz approved these changes Apr 10, 2024

View reviewed changes

ShahabT added 2 commits April 10, 2024 22:24

skip files for 'breaking' check and update openapi files

b3eaacc

remove build_id from TaskQueueVersionInfo

074e9e1

convert types_info into a map

f12a656

dnr approved these changes Apr 10, 2024

View reviewed changes

ShahabT merged commit 2227a14 into master Apr 10, 2024
3 checks passed

ShahabT deleted the versioning-2 branch April 10, 2024 20:39

ShahabT mentioned this pull request Apr 10, 2024

New Worker Versioning API #337

Closed

roxblnfk mentioned this pull request Apr 11, 2024

Expose WorkflowIdConflictPolicy temporalio/sdk-php#417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Versioning 2 API #393

Versioning 2 API #393

ShahabT commented Apr 9, 2024 •

edited

Loading

cretz left a comment •

edited

Loading

cretz Apr 9, 2024

ShahabT Apr 9, 2024

cretz Apr 10, 2024

dnr Apr 9, 2024

ShahabT Apr 10, 2024

dnr Apr 10, 2024

dnr Apr 9, 2024

ShahabT Apr 10, 2024

dnr Apr 9, 2024

ShahabT Apr 9, 2024

dnr Apr 9, 2024

ShahabT Apr 10, 2024

dnr Apr 10, 2024

ShahabT Apr 10, 2024

dnr Apr 10, 2024

dnr Apr 9, 2024

ShahabT commented Apr 10, 2024

cretz left a comment

dnr Apr 10, 2024

Versioning 2 API #393

Versioning 2 API #393

Conversation

ShahabT commented Apr 9, 2024 • edited Loading

cretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShahabT commented Apr 10, 2024

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShahabT commented Apr 9, 2024 •

edited

Loading

cretz left a comment •

edited

Loading